Flume
  1. Flume
  2. FLUME-1019

Document Sink and related interfaces, defining expected behaviors

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: v1.0.0, v1.1.0
    • Fix Version/s: v1.1.0
    • Component/s: Sinks+Sources
    • Labels:
      None

      Description

      Currently the Sink interface is undocumented and some of the other public interfaces that might be implemented by users for custom sinks/processors are poorly documented.

      In the process of documenting these interfaces, I hope to get input from others on what our expected behavior is.

      1. FLUME-1019.2.patch
        6 kB
        Juhani Connolly

        Issue Links

          Activity

          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-03-09 09:16:51, Mike Percy wrote:

          > +1 looks great! I think we should attach this to the JIRA and get it committed.

          >

          > Will, I think we should open a separate JIRA to document the thread safety guarantees. Is that alright with you? Agreed that it's important to document, but this is already a win and the thread safety guarantees require careful attention and affect more areas than just the Sink-related interfaces.

          >

          > Best,

          > Mike

          Ok, agreed.

          • Will

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4175/#review5775
          -----------------------------------------------------------

          On 2012-03-09 07:51:16, Juhani Connolly wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/4175/

          -----------------------------------------------------------

          (Updated 2012-03-09 07:51:16)

          Review request for Flume.

          Summary

          -------

          An initial pass at documenting the interfaces.

          Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors.

          This addresses bug FLUME-1019.

          https://issues.apache.org/jira/browse/FLUME-1019

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e

          flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6

          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56

          Diff: https://reviews.apache.org/r/4175/diff

          Testing

          -------

          No changes have been made to code

          Thanks,

          Juhani

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-03-09 09:16:51, Mike Percy wrote: > +1 looks great! I think we should attach this to the JIRA and get it committed. > > Will, I think we should open a separate JIRA to document the thread safety guarantees. Is that alright with you? Agreed that it's important to document, but this is already a win and the thread safety guarantees require careful attention and affect more areas than just the Sink-related interfaces. > > Best, > Mike Ok, agreed. Will ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/#review5775 ----------------------------------------------------------- On 2012-03-09 07:51:16, Juhani Connolly wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/ ----------------------------------------------------------- (Updated 2012-03-09 07:51:16) Review request for Flume. Summary ------- An initial pass at documenting the interfaces. Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors. This addresses bug FLUME-1019 . https://issues.apache.org/jira/browse/FLUME-1019 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6 flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56 Diff: https://reviews.apache.org/r/4175/diff Testing ------- No changes have been made to code Thanks, Juhani
          Arvind Prabhakar made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Arvind Prabhakar added a comment -

          Patch committed. Thanks Juhani!

          Show
          Arvind Prabhakar added a comment - Patch committed. Thanks Juhani!
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4175/#review5781
          -----------------------------------------------------------

          Ship it!

          +1

          • Arvind

          On 2012-03-09 07:51:16, Juhani Connolly wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/4175/

          -----------------------------------------------------------

          (Updated 2012-03-09 07:51:16)

          Review request for Flume.

          Summary

          -------

          An initial pass at documenting the interfaces.

          Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors.

          This addresses bug FLUME-1019.

          https://issues.apache.org/jira/browse/FLUME-1019

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e

          flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6

          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56

          Diff: https://reviews.apache.org/r/4175/diff

          Testing

          -------

          No changes have been made to code

          Thanks,

          Juhani

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/#review5781 ----------------------------------------------------------- Ship it! +1 Arvind On 2012-03-09 07:51:16, Juhani Connolly wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/ ----------------------------------------------------------- (Updated 2012-03-09 07:51:16) Review request for Flume. Summary ------- An initial pass at documenting the interfaces. Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors. This addresses bug FLUME-1019 . https://issues.apache.org/jira/browse/FLUME-1019 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6 flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56 Diff: https://reviews.apache.org/r/4175/diff Testing ------- No changes have been made to code Thanks, Juhani
          Juhani Connolly made changes -
          Attachment FLUME-1019.patch [ 12517347 ]
          Juhani Connolly made changes -
          Attachment FLUME-1019.2.patch [ 12517702 ]
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4175/#review5775
          -----------------------------------------------------------

          Ship it!

          +1 looks great! I think we should attach this to the JIRA and get it committed.

          Will, I think we should open a separate JIRA to document the thread safety guarantees. Is that alright with you? Agreed that it's important to document, but this is already a win and the thread safety guarantees require careful attention and affect more areas than just the Sink-related interfaces.

          Best,
          Mike

          • Mike

          On 2012-03-09 07:51:16, Juhani Connolly wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/4175/

          -----------------------------------------------------------

          (Updated 2012-03-09 07:51:16)

          Review request for Flume.

          Summary

          -------

          An initial pass at documenting the interfaces.

          Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors.

          This addresses bug FLUME-1019.

          https://issues.apache.org/jira/browse/FLUME-1019

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e

          flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6

          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56

          Diff: https://reviews.apache.org/r/4175/diff

          Testing

          -------

          No changes have been made to code

          Thanks,

          Juhani

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/#review5775 ----------------------------------------------------------- Ship it! +1 looks great! I think we should attach this to the JIRA and get it committed. Will, I think we should open a separate JIRA to document the thread safety guarantees. Is that alright with you? Agreed that it's important to document, but this is already a win and the thread safety guarantees require careful attention and affect more areas than just the Sink-related interfaces. Best, Mike Mike On 2012-03-09 07:51:16, Juhani Connolly wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/ ----------------------------------------------------------- (Updated 2012-03-09 07:51:16) Review request for Flume. Summary ------- An initial pass at documenting the interfaces. Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors. This addresses bug FLUME-1019 . https://issues.apache.org/jira/browse/FLUME-1019 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6 flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56 Diff: https://reviews.apache.org/r/4175/diff Testing ------- No changes have been made to code Thanks, Juhani
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-03-08 00:32:59, Mike Percy wrote:

          > Hi Juhani, sorry for the delay in getting back to you on this. I took a look at the added docs and it looks great. I think this type of documentation really helps to clarify how the system is supposed to act.

          >

          > I added a few suggestions below.

          Will McQueen wrote:

          Hi Juhani,

          How about adding some concurrency-related comments or annotations (eg, JCIP-style, EJ-style or JSR-305 style... I'm not sure which one we've standardized on for annotations, but I've seen some somewhere in the code before) so that users know whether to make their custom implementations of components (such as Sink) threadsafe, and so devs can tell at a glance whether any future changes to the impl need to honor a specific thread safety contract? EJ 2nd edition has Item 70: "Document thread safety". It recommends, "To enable safe concurrent use, a class must clearly document what level of thread safety it supports".

          Cheers,

          Will

          Good point. I'll have to do this after the weekend but I'll get on it!

          • Juhani

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4175/#review5698
          -----------------------------------------------------------

          On 2012-03-09 07:51:16, Juhani Connolly wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/4175/

          -----------------------------------------------------------

          (Updated 2012-03-09 07:51:16)

          Review request for Flume.

          Summary

          -------

          An initial pass at documenting the interfaces.

          Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors.

          This addresses bug FLUME-1019.

          https://issues.apache.org/jira/browse/FLUME-1019

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e

          flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6

          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56

          Diff: https://reviews.apache.org/r/4175/diff

          Testing

          -------

          No changes have been made to code

          Thanks,

          Juhani

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-03-08 00:32:59, Mike Percy wrote: > Hi Juhani, sorry for the delay in getting back to you on this. I took a look at the added docs and it looks great. I think this type of documentation really helps to clarify how the system is supposed to act. > > I added a few suggestions below. Will McQueen wrote: Hi Juhani, How about adding some concurrency-related comments or annotations (eg, JCIP-style, EJ-style or JSR-305 style... I'm not sure which one we've standardized on for annotations, but I've seen some somewhere in the code before) so that users know whether to make their custom implementations of components (such as Sink) threadsafe, and so devs can tell at a glance whether any future changes to the impl need to honor a specific thread safety contract? EJ 2nd edition has Item 70: "Document thread safety". It recommends, "To enable safe concurrent use, a class must clearly document what level of thread safety it supports". Cheers, Will Good point. I'll have to do this after the weekend but I'll get on it! Juhani ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/#review5698 ----------------------------------------------------------- On 2012-03-09 07:51:16, Juhani Connolly wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/ ----------------------------------------------------------- (Updated 2012-03-09 07:51:16) Review request for Flume. Summary ------- An initial pass at documenting the interfaces. Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors. This addresses bug FLUME-1019 . https://issues.apache.org/jira/browse/FLUME-1019 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6 flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56 Diff: https://reviews.apache.org/r/4175/diff Testing ------- No changes have been made to code Thanks, Juhani
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-03-08 00:32:59, Mike Percy wrote:

          > flume-ng-core/src/main/java/org/apache/flume/Sink.java, line 60

          > <https://reviews.apache.org/r/4175/diff/1/?file=88130#file88130line60>

          >

          > I think this part is a little vague. How about this?

          >

          > @return Returns READY if 1 or more events were successfully delivered, or BACKOFF if no events could be retrieved from the channel feeding this sink.

          sounds good

          On 2012-03-08 00:32:59, Mike Percy wrote:

          > flume-ng-core/src/main/java/org/apache/flume/Sink.java, line 62

          > <https://reviews.apache.org/r/4175/diff/1/?file=88130#file88130line62>

          >

          > Maybe we should say:

          >

          > @throws EventDeliveryException if there is a failure delivering any Event to the next-hop destination.

          >

          > (final destination is not guaranteed)

          yeah, I see how that could be misunderstood, I had meant it as final iff the sink was something like a file sink with no further hops. Taking that out

          On 2012-03-08 00:32:59, Mike Percy wrote:

          > flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java, line 31

          > <https://reviews.apache.org/r/4175/diff/1/?file=88131#file88131line31>

          >

          > Missing a component of the package: org.apache.flume.sink.SinkGroup

          oops.
          We may want to review which packages belong in org.apache.flume, I think some of them don't belong there but in a sub-package. That's an issue for another jira though

          On 2012-03-08 00:32:59, Mike Percy wrote:

          > flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java, line 37

          > <https://reviews.apache.org/r/4175/diff/1/?file=88131#file88131line37>

          >

          > That is an improvement but I think it kind of refers to the failover sink processor's delivery policy (w.r.t. throwing the exception). Maybe something like this would be more general?

          >

          > The processor is expected to call {@linkplain Sink#process()} on whatever sink(s) appropriate, handling failures as appropriate and throwing {@link EventDeliveryException} when there is a failure to deliver any events according to the delivery policy defined by the sink processor implementation. See specific implementations of this interface for delivery behavior and policies.

          >

          Much better, thanks

          On 2012-03-08 00:32:59, Mike Percy wrote:

          > flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java, line 41

          > <https://reviews.apache.org/r/4175/diff/1/?file=88131#file88131line41>

          >

          > These old constants should be corrected. How about:

          >

          > @return Returns {@code READY} if events were successfully consumed, or {@code BACKOFF} if no events were available in the channel to consume.

          >

          > @throws EventDeliveryException if any failure to deliver events occurred

          Done. I changed the @throws a bit to refer to individual processor behaviors

          • Juhani

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4175/#review5698
          -----------------------------------------------------------

          On 2012-03-05 09:30:21, Juhani Connolly wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/4175/

          -----------------------------------------------------------

          (Updated 2012-03-05 09:30:21)

          Review request for Flume.

          Summary

          -------

          An initial pass at documenting the interfaces.

          Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors.

          This addresses bug FLUME-1019.

          https://issues.apache.org/jira/browse/FLUME-1019

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6

          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e

          Diff: https://reviews.apache.org/r/4175/diff

          Testing

          -------

          No changes have been made to code

          Thanks,

          Juhani

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-03-08 00:32:59, Mike Percy wrote: > flume-ng-core/src/main/java/org/apache/flume/Sink.java, line 60 > < https://reviews.apache.org/r/4175/diff/1/?file=88130#file88130line60 > > > I think this part is a little vague. How about this? > > @return Returns READY if 1 or more events were successfully delivered, or BACKOFF if no events could be retrieved from the channel feeding this sink. sounds good On 2012-03-08 00:32:59, Mike Percy wrote: > flume-ng-core/src/main/java/org/apache/flume/Sink.java, line 62 > < https://reviews.apache.org/r/4175/diff/1/?file=88130#file88130line62 > > > Maybe we should say: > > @throws EventDeliveryException if there is a failure delivering any Event to the next-hop destination. > > (final destination is not guaranteed) yeah, I see how that could be misunderstood, I had meant it as final iff the sink was something like a file sink with no further hops. Taking that out On 2012-03-08 00:32:59, Mike Percy wrote: > flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java, line 31 > < https://reviews.apache.org/r/4175/diff/1/?file=88131#file88131line31 > > > Missing a component of the package: org.apache.flume.sink.SinkGroup oops. We may want to review which packages belong in org.apache.flume, I think some of them don't belong there but in a sub-package. That's an issue for another jira though On 2012-03-08 00:32:59, Mike Percy wrote: > flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java, line 37 > < https://reviews.apache.org/r/4175/diff/1/?file=88131#file88131line37 > > > That is an improvement but I think it kind of refers to the failover sink processor's delivery policy (w.r.t. throwing the exception). Maybe something like this would be more general? > > The processor is expected to call {@linkplain Sink#process()} on whatever sink(s) appropriate, handling failures as appropriate and throwing {@link EventDeliveryException} when there is a failure to deliver any events according to the delivery policy defined by the sink processor implementation. See specific implementations of this interface for delivery behavior and policies. > Much better, thanks On 2012-03-08 00:32:59, Mike Percy wrote: > flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java, line 41 > < https://reviews.apache.org/r/4175/diff/1/?file=88131#file88131line41 > > > These old constants should be corrected. How about: > > @return Returns {@code READY} if events were successfully consumed, or {@code BACKOFF} if no events were available in the channel to consume. > > @throws EventDeliveryException if any failure to deliver events occurred Done. I changed the @throws a bit to refer to individual processor behaviors Juhani ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/#review5698 ----------------------------------------------------------- On 2012-03-05 09:30:21, Juhani Connolly wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/ ----------------------------------------------------------- (Updated 2012-03-05 09:30:21) Review request for Flume. Summary ------- An initial pass at documenting the interfaces. Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors. This addresses bug FLUME-1019 . https://issues.apache.org/jira/browse/FLUME-1019 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6 flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56 flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e Diff: https://reviews.apache.org/r/4175/diff Testing ------- No changes have been made to code Thanks, Juhani
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-03-08 00:32:59, Mike Percy wrote:

          > Hi Juhani, sorry for the delay in getting back to you on this. I took a look at the added docs and it looks great. I think this type of documentation really helps to clarify how the system is supposed to act.

          >

          > I added a few suggestions below.

          Hi Juhani,

          How about adding some concurrency-related comments or annotations (eg, JCIP-style, EJ-style or JSR-305 style... I'm not sure which one we've standardized on for annotations, but I've seen some somewhere in the code before) so that users know whether to make their custom implementations of components (such as Sink) threadsafe, and so devs can tell at a glance whether any future changes to the impl need to honor a specific thread safety contract? EJ 2nd edition has Item 70: "Document thread safety". It recommends, "To enable safe concurrent use, a class must clearly document what level of thread safety it supports".

          Cheers,
          Will

          • Will

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4175/#review5698
          -----------------------------------------------------------

          On 2012-03-09 07:51:16, Juhani Connolly wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/4175/

          -----------------------------------------------------------

          (Updated 2012-03-09 07:51:16)

          Review request for Flume.

          Summary

          -------

          An initial pass at documenting the interfaces.

          Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors.

          This addresses bug FLUME-1019.

          https://issues.apache.org/jira/browse/FLUME-1019

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e

          flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6

          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56

          Diff: https://reviews.apache.org/r/4175/diff

          Testing

          -------

          No changes have been made to code

          Thanks,

          Juhani

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-03-08 00:32:59, Mike Percy wrote: > Hi Juhani, sorry for the delay in getting back to you on this. I took a look at the added docs and it looks great. I think this type of documentation really helps to clarify how the system is supposed to act. > > I added a few suggestions below. Hi Juhani, How about adding some concurrency-related comments or annotations (eg, JCIP-style, EJ-style or JSR-305 style... I'm not sure which one we've standardized on for annotations, but I've seen some somewhere in the code before) so that users know whether to make their custom implementations of components (such as Sink) threadsafe, and so devs can tell at a glance whether any future changes to the impl need to honor a specific thread safety contract? EJ 2nd edition has Item 70: "Document thread safety". It recommends, "To enable safe concurrent use, a class must clearly document what level of thread safety it supports". Cheers, Will Will ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/#review5698 ----------------------------------------------------------- On 2012-03-09 07:51:16, Juhani Connolly wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/ ----------------------------------------------------------- (Updated 2012-03-09 07:51:16) Review request for Flume. Summary ------- An initial pass at documenting the interfaces. Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors. This addresses bug FLUME-1019 . https://issues.apache.org/jira/browse/FLUME-1019 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6 flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56 Diff: https://reviews.apache.org/r/4175/diff Testing ------- No changes have been made to code Thanks, Juhani
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4175/
          -----------------------------------------------------------

          (Updated 2012-03-09 07:51:16.055378)

          Review request for Flume.

          Changes
          -------

          Made the suggested changes

          Summary
          -------

          An initial pass at documenting the interfaces.
          Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors.

          This addresses bug FLUME-1019.
          https://issues.apache.org/jira/browse/FLUME-1019

          Diffs (updated)


          flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e
          flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6
          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad
          flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56

          Diff: https://reviews.apache.org/r/4175/diff

          Testing
          -------

          No changes have been made to code

          Thanks,

          Juhani

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/ ----------------------------------------------------------- (Updated 2012-03-09 07:51:16.055378) Review request for Flume. Changes ------- Made the suggested changes Summary ------- An initial pass at documenting the interfaces. Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors. This addresses bug FLUME-1019 . https://issues.apache.org/jira/browse/FLUME-1019 Diffs (updated) flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6 flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56 Diff: https://reviews.apache.org/r/4175/diff Testing ------- No changes have been made to code Thanks, Juhani
          Mike Percy made changes -
          Link This issue relates to FLUME-1021 [ FLUME-1021 ]
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4175/#review5698
          -----------------------------------------------------------

          Hi Juhani, sorry for the delay in getting back to you on this. I took a look at the added docs and it looks great. I think this type of documentation really helps to clarify how the system is supposed to act.

          I added a few suggestions below.

          flume-ng-core/src/main/java/org/apache/flume/Sink.java
          <https://reviews.apache.org/r/4175/#comment12403>

          I think this part is a little vague. How about this?

          @return Returns READY if 1 or more events were successfully delivered, or BACKOFF if no events could be retrieved from the channel feeding this sink.

          flume-ng-core/src/main/java/org/apache/flume/Sink.java
          <https://reviews.apache.org/r/4175/#comment12408>

          Maybe we should say:

          @throws EventDeliveryException if there is a failure delivering any Event to the next-hop destination.

          (final destination is not guaranteed)

          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java
          <https://reviews.apache.org/r/4175/#comment12404>

          Missing a component of the package: org.apache.flume.sink.SinkGroup

          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java
          <https://reviews.apache.org/r/4175/#comment12405>

          That is an improvement but I think it kind of refers to the failover sink processor's delivery policy (w.r.t. throwing the exception). Maybe something like this would be more general?

          The processor is expected to call

          {@linkplain Sink#process()}

          on whatever sink(s) appropriate, handling failures as appropriate and throwing

          {@link EventDeliveryException}

          when there is a failure to deliver any events according to the delivery policy defined by the sink processor implementation. See specific implementations of this interface for delivery behavior and policies.

          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java
          <https://reviews.apache.org/r/4175/#comment12407>

          These old constants should be corrected. How about:

          @return Returns

          {@code READY}

          if events were successfully consumed, or

          {@code BACKOFF}

          if no events were available in the channel to consume.

          @throws EventDeliveryException if any failure to deliver events occurred

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java
          <https://reviews.apache.org/r/4175/#comment12406>

          may want to remove this whitespace

          • Mike

          On 2012-03-05 09:30:21, Juhani Connolly wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/4175/

          -----------------------------------------------------------

          (Updated 2012-03-05 09:30:21)

          Review request for Flume.

          Summary

          -------

          An initial pass at documenting the interfaces.

          Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors.

          This addresses bug FLUME-1019.

          https://issues.apache.org/jira/browse/FLUME-1019

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6

          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56

          flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e

          Diff: https://reviews.apache.org/r/4175/diff

          Testing

          -------

          No changes have been made to code

          Thanks,

          Juhani

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/#review5698 ----------------------------------------------------------- Hi Juhani, sorry for the delay in getting back to you on this. I took a look at the added docs and it looks great. I think this type of documentation really helps to clarify how the system is supposed to act. I added a few suggestions below. flume-ng-core/src/main/java/org/apache/flume/Sink.java < https://reviews.apache.org/r/4175/#comment12403 > I think this part is a little vague. How about this? @return Returns READY if 1 or more events were successfully delivered, or BACKOFF if no events could be retrieved from the channel feeding this sink. flume-ng-core/src/main/java/org/apache/flume/Sink.java < https://reviews.apache.org/r/4175/#comment12408 > Maybe we should say: @throws EventDeliveryException if there is a failure delivering any Event to the next-hop destination. (final destination is not guaranteed) flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java < https://reviews.apache.org/r/4175/#comment12404 > Missing a component of the package: org.apache.flume.sink.SinkGroup flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java < https://reviews.apache.org/r/4175/#comment12405 > That is an improvement but I think it kind of refers to the failover sink processor's delivery policy (w.r.t. throwing the exception). Maybe something like this would be more general? The processor is expected to call {@linkplain Sink#process()} on whatever sink(s) appropriate, handling failures as appropriate and throwing {@link EventDeliveryException} when there is a failure to deliver any events according to the delivery policy defined by the sink processor implementation. See specific implementations of this interface for delivery behavior and policies. flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java < https://reviews.apache.org/r/4175/#comment12407 > These old constants should be corrected. How about: @return Returns {@code READY} if events were successfully consumed, or {@code BACKOFF} if no events were available in the channel to consume. @throws EventDeliveryException if any failure to deliver events occurred flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java < https://reviews.apache.org/r/4175/#comment12406 > may want to remove this whitespace Mike On 2012-03-05 09:30:21, Juhani Connolly wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/ ----------------------------------------------------------- (Updated 2012-03-05 09:30:21) Review request for Flume. Summary ------- An initial pass at documenting the interfaces. Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors. This addresses bug FLUME-1019 . https://issues.apache.org/jira/browse/FLUME-1019 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6 flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56 flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e Diff: https://reviews.apache.org/r/4175/diff Testing ------- No changes have been made to code Thanks, Juhani
          Juhani Connolly made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Juhani Connolly made changes -
          Field Original Value New Value
          Attachment FLUME-1019.patch [ 12517347 ]
          Hide
          Juhani Connolly added a comment -

          Same patch that was put up for review

          Show
          Juhani Connolly added a comment - Same patch that was put up for review
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4175/
          -----------------------------------------------------------

          Review request for Flume.

          Summary
          -------

          An initial pass at documenting the interfaces.
          Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors.

          This addresses bug FLUME-1019.
          https://issues.apache.org/jira/browse/FLUME-1019

          Diffs


          flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6
          flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad
          flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56
          flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e

          Diff: https://reviews.apache.org/r/4175/diff

          Testing
          -------

          No changes have been made to code

          Thanks,

          Juhani

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4175/ ----------------------------------------------------------- Review request for Flume. Summary ------- An initial pass at documenting the interfaces. Let me know if I missed anything relevant, or if you feel that this does not correctly represent our expected behaviors. This addresses bug FLUME-1019 . https://issues.apache.org/jira/browse/FLUME-1019 Diffs flume-ng-core/src/main/java/org/apache/flume/Sink.java 3abeeb6 flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java 11651ad flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 2e80a56 flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 10f9f4e Diff: https://reviews.apache.org/r/4175/diff Testing ------- No changes have been made to code Thanks, Juhani
          Juhani Connolly created issue -

            People

            • Assignee:
              Juhani Connolly
              Reporter:
              Juhani Connolly
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development