Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None

      Description

      The ChukwaWriter interface has both add(Chunk) and add(List<Chunk>). The code doesn't actually use the former. I'd like to remove it. Thoughts?

        Issue Links

          Activity

          Hide
          Ari Rabkin added a comment -

          I don't want to hack at the ChukwaWriter API while there's an outstanding patch that relies on it – but if we go through another draft for HADOOP-5018, I'd be tempted to incorporate this.

          Show
          Ari Rabkin added a comment - I don't want to hack at the ChukwaWriter API while there's an outstanding patch that relies on it – but if we go through another draft for HADOOP-5018 , I'd be tempted to incorporate this.
          Hide
          Eric Yang added a comment -

          +1 for remove add(Chunk).

          Show
          Eric Yang added a comment - +1 for remove add(Chunk).
          Hide
          Ari Rabkin added a comment -

          The other option, as I think about it, is to ditch the list version and only have add(Chunk). The one class where add(List) is different is that in SeqFileWriter, it caches the computation of the time partition. But it might be sensible to just push that into a timer, and have only the simpler add method.

          There's no interesting semantic behind which Chunks go in the same list --an HTTP post to the collector doesn't have interesting semantics. So ditching add(List<>) will prevent mistaken assumptions.

          Show
          Ari Rabkin added a comment - The other option, as I think about it, is to ditch the list version and only have add(Chunk). The one class where add(List) is different is that in SeqFileWriter, it caches the computation of the time partition. But it might be sensible to just push that into a timer, and have only the simpler add method. There's no interesting semantic behind which Chunks go in the same list --an HTTP post to the collector doesn't have interesting semantics. So ditching add(List<>) will prevent mistaken assumptions.
          Hide
          Jerome Boulon added a comment - - edited

          >> an HTTP post to the collector doesn't have interesting semantics. So ditching add(List<>) will prevent mistaken assumptions.
          No totally true.
          The ChukwaHTTPSender is an implementation of the ChukwaSender interface and if you were using a JDBCCollector you will open a transaction, insert the list of chunks in batch mode then commit. I think that it could be the same for an HTTP post.
          It best to be able to acknowledge at the HTTP post level rather than at each individual chunk, at least it gave us the flexibility to do it at this level.

          So +1 for remove add(Chunk) and not ditching add(List<>) .

          The next question is do we want to report any warning/error in a well defined Object so the collector can report that in a unified way.
          Right now the HTTPSender is not using the HTTP response.

          Show
          Jerome Boulon added a comment - - edited >> an HTTP post to the collector doesn't have interesting semantics. So ditching add(List<>) will prevent mistaken assumptions. No totally true. The ChukwaHTTPSender is an implementation of the ChukwaSender interface and if you were using a JDBCCollector you will open a transaction, insert the list of chunks in batch mode then commit. I think that it could be the same for an HTTP post. It best to be able to acknowledge at the HTTP post level rather than at each individual chunk, at least it gave us the flexibility to do it at this level. So +1 for remove add(Chunk) and not ditching add(List<>) . The next question is do we want to report any warning/error in a well defined Object so the collector can report that in a unified way. Right now the HTTPSender is not using the HTTP response.
          Hide
          Ari Rabkin added a comment -

          I'm convinced by the point about transactions. So add(Chunk) will go, and add(List) will stay.

          As to warnings or errors – this is in the context of Writers, on the collector side. So there's an easy error reporting mechanism: the writer should throw a WriterException if it isn't willing to take responsibility for the chunks in the list.

          Show
          Ari Rabkin added a comment - I'm convinced by the point about transactions. So add(Chunk) will go, and add(List) will stay. As to warnings or errors – this is in the context of Writers, on the collector side. So there's an easy error reporting mechanism: the writer should throw a WriterException if it isn't willing to take responsibility for the chunks in the list.
          Hide
          Ari Rabkin added a comment -

          Cleans up interfaces by removing add(Chunk)

          Show
          Ari Rabkin added a comment - Cleans up interfaces by removing add(Chunk)
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12400401/writerInterface.patch
          against trunk revision 745268.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 4 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3877/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3877/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3877/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3877/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12400401/writerInterface.patch against trunk revision 745268. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3877/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3877/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3877/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3877/console This message is automatically generated.
          Hide
          Ari Rabkin added a comment -

          I just committed this,

          Show
          Ari Rabkin added a comment - I just committed this,
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk #766 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/766/)
          Simplify Chukwa writer API.
          Contributed by Ari Rabkin (asrabkin)

          Show
          Hudson added a comment - Integrated in Hadoop-trunk #766 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/766/ ) Simplify Chukwa writer API. Contributed by Ari Rabkin (asrabkin)
          Hide
          Robert Chansler added a comment -

          Editorial pass over all release notes prior to publication of 0.21. Routine.

          Show
          Robert Chansler added a comment - Editorial pass over all release notes prior to publication of 0.21. Routine.

            People

            • Assignee:
              Ari Rabkin
              Reporter:
              Ari Rabkin
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development