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

Review request for Flume.
Description
-------

Initial implementation of Thrift RPC client. I will add some comments on why the exception handling is done by catching and sending the errors as part of the result.  I had to change some of the pom.xml files which generate thrift java files, because the mvn-thrift plugin is no longer on maven. So, changed the generation code to the one I used. Generating the code requires thrift 0.6.1 - since there is no .thrift file for scribe sources, and thrift generated code is backwards incompatible between 0.9 and 0.6.1.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs
-----

  flume-ng-channels/flume-file-channel/pom.xml 2408447
  flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java d921387
  flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java 8a4201c
  flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFileChannelRestart.java ea57cdb
  flume-ng-core/pom.xml ba414bc
  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/thrift/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/thrift/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/thrift/ThriftTestingSource.java PRE-CREATION
  flume-ng-sinks/flume-hdfs-sink/pom.xml aaa0e1f
  flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java 3f31ef2
  flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java 8bf0509
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  pom.xml 5d59d44

Diff: https://reviews.apache.org/r/9283/diff/
Testing
-------

Added some unit tests.
Thanks,

Hari Shreedharan

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

Review request for Flume.
Description
-------

Added thrift rpc client. Detailed description posted on jira.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs
-----

  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/thrift/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Result.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/thrift/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/thrift/ThriftTestingSource.java PRE-CREATION
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  pom.xml faa67f9

Diff: https://reviews.apache.org/r/9284/diff/
Testing
-------

Added unit tests for new code.
Thanks,

Hari Shreedharan

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

(Updated Feb. 4, 2013, 9:02 a.m.)
Review request for Flume.
Description
-------

Added thrift rpc client. Detailed description posted on jira.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs (updated)
-----

  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
  flume-ng-sdk/src/main/java/org/apache/flume/api/thrift/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Result.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/thrift/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/thrift/ThriftTestingSource.java PRE-CREATION
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  pom.xml faa67f9

Diff: https://reviews.apache.org/r/9284/diff/
Testing
-------

Added unit tests for new code.
Thanks,

Hari Shreedharan

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

(Updated Feb. 6, 2013, 7:15 a.m.)
Review request for Flume.
Changes
-------

Support pooling connections, and fix legacy thrift source and scribe source pom.xml to build with Thrift 0.9.0
Description
-------

Added thrift rpc client. Detailed description posted on jira.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs (updated)
-----

  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
  flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
  flume-ng-sdk/src/main/java/org/apache/flume/api/thrift/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Result.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/thrift/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/thrift/ThriftTestingSource.java PRE-CREATION
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
  flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
  pom.xml faa67f9

Diff: https://reviews.apache.org/r/9284/diff/
Testing
-------

Added unit tests for new code.
Thanks,

Hari Shreedharan

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

(Updated Feb. 6, 2013, 7:42 a.m.)
Review request for Flume.
Changes
-------

Add hash code generation.
Description
-------

Added thrift rpc client. Detailed description posted on jira.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs (updated)
-----

  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
  flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
  flume-ng-sdk/src/main/java/org/apache/flume/api/thrift/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Result.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/thrift/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/thrift/ThriftTestingSource.java PRE-CREATION
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
  flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
  pom.xml faa67f9

Diff: https://reviews.apache.org/r/9284/diff/
Testing
-------

Added unit tests for new code.
Thanks,

Hari Shreedharan

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

(Updated Feb. 7, 2013, 2:01 a.m.)
Review request for Flume.
Changes
-------

Added Factory methods, removed an old file which was Thrift generated.
Description
-------

Added thrift rpc client. Detailed description posted on jira.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs (updated)
-----

  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
  flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
  flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
  flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
  pom.xml faa67f9

Diff: https://reviews.apache.org/r/9284/diff/
Testing
-------

Added unit tests for new code.
Thanks,

Hari Shreedharan

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

(Updated Feb. 7, 2013, 3:31 a.m.)
Review request for Flume.
Changes
-------

Use TFastFramedTransport and TCompactProtocol.
Description
-------

Added thrift rpc client. Detailed description posted on jira.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs (updated)
-----

  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
  flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
  flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
  flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
  pom.xml faa67f9

Diff: https://reviews.apache.org/r/9284/diff/
Testing
-------

Added unit tests for new code.
Thanks,

Hari Shreedharan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16274
-----------------------------------------------------------
Thanks Hari! Looks good! A few comments below. I'll do another review later.
flume-ng-legacy-sources/flume-thrift-source/pom.xml
<https://reviews.apache.org/r/9284/#comment34729>

    If this fails we do we want to print the error and then exit?

flume-ng-legacy-sources/flume-thrift-source/pom.xml
<https://reviews.apache.org/r/9284/#comment34730>

    same here?

flume-ng-sdk/pom.xml
<https://reviews.apache.org/r/9284/#comment34731>

    Same questions on this script

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34732>

    nit: Looks like the variables in the constructor can be set to final

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34733>

    nit: it's nice to have this up at the top of the class

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34734>

    Is this meant to be e.getCause() instance of?
    
    I don't follow the layers of instanceof.

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34735>

    Any throwable not matching the above is turned into an EventDeliveryException including Error and RuntimeException. I don't think we should do that for runtime errors.

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34736>

    Not a huge deal, but we do this "Exception follows" messages all over flume. I don't really see a purpose in that part of the text?

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34737>

    Same exception stuff as above

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34738>

    remove sysout

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34741>

    Looks like there are more than two stati, if so I think we should include the status in the error message.

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34739>

    remove sysout

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34740>

    converting all throwables to FlumeException should be handled better
- Brock Noland
On Feb. 7, 2013, 3:31 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2013, 3:31 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5

> On Feb. 7, 2013, 3:40 p.m., Brock Noland wrote:
> > Thanks Hari! Looks good! A few comments below. I'll do another review later.

Hi Brock,

Thanks for the review. It looks like you reviewed revision 5 of the patch, but there is a newer version of the patch. Most of your concerns are valid even in that patch too, except the sysout ones - which were removed.
> On Feb. 7, 2013, 3:40 p.m., Brock Noland wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 218
> > <https://reviews.apache.org/r/9284/diff/5/?file=256437#file256437line218>
> >
> >     converting all throwables to FlumeException should be handled better

Can you please elaborate on this? Do you mean throwing errors as it is, without wrapping in FlumeException?
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16274
-----------------------------------------------------------
On Feb. 7, 2013, 3:31 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2013, 3:31 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
>   flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
>   flume-ng-sources/flume-scribe-source/pom.xml 588207d
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
>   flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
>   pom.xml faa67f9
>
> Diff: https://reviews.apache.org/r/9284/diff/
>
>
> Testing
> -------
>
> Added unit tests for new code.
>
>
> Thanks,
>
> Hari Shreedharan

> On Feb. 7, 2013, 3:40 p.m., Brock Noland wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 218
> > <https://reviews.apache.org/r/9284/diff/5/?file=256437#file256437line218>
> >
> >     converting all throwables to FlumeException should be handled better
>
> Hari Shreedharan wrote:
>     Can you please elaborate on this? Do you mean throwing errors as it is, without wrapping in FlumeException?

Sorry, I mean that Throwable catches everything. I don't think we should wrap things like OutOfMemoryError or NullPointerException into FlumeException.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16274
-----------------------------------------------------------
On Feb. 7, 2013, 3:31 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2013, 3:31 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
>   flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
>   flume-ng-sources/flume-scribe-source/pom.xml 588207d
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
>   flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
>   pom.xml faa67f9
>
> Diff: https://reviews.apache.org/r/9284/diff/
>
>
> Testing
> -------
>
> Added unit tests for new code.
>
>
> Thanks,
>
> Hari Shreedharan
>
>

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

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34750>

    Yep, good catch!

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34751>

    Agreed, will update.

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34752>

    Sure.
- Hari Shreedharan
On Feb. 7, 2013, 3:31 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2013, 3:31 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
>   flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
>   flume-ng-sources/flume-scribe-source/pom.xml 588207d
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
>   flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
>   pom.xml faa67f9
>
> Diff: https://reviews.apache.org/r/9284/diff/
>
>
> Testing
> -------
>
> Added unit tests for new code.
>
>
> Thanks,
>
> Hari Shreedharan
>
>

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

(Updated Feb. 7, 2013, 9:31 p.m.)
Review request for Flume.
Changes
-------

Incorporating Brock's feedback.
Description
-------

Added thrift rpc client. Detailed description posted on jira.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs (updated)
-----

  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
  flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
  flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
  flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
  pom.xml faa67f9

Diff: https://reviews.apache.org/r/9284/diff/
Testing
-------

Added unit tests for new code.
Thanks,

Hari Shreedharan

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

(Updated Feb. 8, 2013, 9:24 a.m.)
Review request for Flume.
Changes
-------

Some extra methods added for Thrift Sink testing. RB would not allow me to modify files not in the repo, so adding these changes into this patch.
Description
-------

Added thrift rpc client. Detailed description posted on jira.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs (updated)
-----

  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
  flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
  flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
  flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
  pom.xml faa67f9

Diff: https://reviews.apache.org/r/9284/diff/
Testing
-------

Added unit tests for new code.
Thanks,

Hari Shreedharan

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

(Updated Feb. 8, 2013, 10:40 p.m.)
Review request for Flume.
Changes
-------

Making sure the incoming list is not changed.
Description
-------

Added thrift rpc client. Detailed description posted on jira.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs (updated)
-----

  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
  flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
  flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
  flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
  pom.xml faa67f9

Diff: https://reviews.apache.org/r/9284/diff/
Testing
-------

Added unit tests for new code.
Thanks,

Hari Shreedharan

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

(Updated Feb. 8, 2013, 11:05 p.m.)
Review request for Flume.
Changes
-------

Updated patch
Description
-------

Added thrift rpc client. Detailed description posted on jira.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs (updated)
-----

  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
  flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
  flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
  flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
  pom.xml faa67f9

Diff: https://reviews.apache.org/r/9284/diff/
Testing
-------

Added unit tests for new code.
Thanks,

Hari Shreedharan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16373
-----------------------------------------------------------
Hari, looks really good to me. I have a few comments below.
flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2
<https://reviews.apache.org/r/9284/#comment34846>

    Not sure what this is for.

flume-ng-sdk/src/main/thrift/aslv2
<https://reviews.apache.org/r/9284/#comment34844>

    What is the point of this file?

flume-ng-sdk/src/main/thrift/flume.thrift
<https://reviews.apache.org/r/9284/#comment34849>

    What do you think about adding an additional health-check method here? Something like:
    
    Status areYouOk()

flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2
<https://reviews.apache.org/r/9284/#comment34847>

    This one too :)

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34858>

    Should this be remove()? Or we should do a null check.

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34860>

    This is O(n) ... we could make this faster.

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34859>

    This is O(n) ... can we get it down to O(1) or O(log n)?
- Mike Percy
On Feb. 8, 2013, 11:05 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2013, 11:05 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
>   flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
>   flume-ng-sources/flume-scribe-source/pom.xml 588207d
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b

> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2, line 1
> > <https://reviews.apache.org/r/9284/diff/8/?file=256978#file256978line1>
> >
> >     Not sure what this is for.

This is to insert the license into the generated code. See the pom.xml change.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sdk/src/main/thrift/aslv2, line 1
> > <https://reviews.apache.org/r/9284/diff/8/?file=256987#file256987line1>
> >
> >     What is the point of this file?

same as above.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sdk/src/main/thrift/flume.thrift, line 37
> > <https://reviews.apache.org/r/9284/diff/8/?file=256988#file256988line37>
> >
> >     What do you think about adding an additional health-check method here? Something like:
> >    
> >     Status areYouOk()

I don't think that is required. I don't see any value in adding that, if there is a failure, the next RPC will fail rightaway if it is a network issue, any others such as Channel issues cannot be predicted by such a health-check anyway.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2, line 1
> > <https://reviews.apache.org/r/9284/diff/8/?file=256996#file256996line1>
> >
> >     This one too :)

same as above.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 350
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line350>
> >
> >     Should this be remove()? Or we should do a null check.

Neither of these. There is a condition check (availableClient.isEmpty) on which a signal waits, so at this point, availableClient is guaranteed to be non-empty. Doing the condition.await and signal allows us to do lazy initialization.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 362
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line362>
> >
> >     This is O(n) ... we could make this faster.

If checkedOutClients is small enough, this is pretty fast. That said, I think we can make checkedOutClients a LinkedHashSet I guess.
> On Feb. 9, 2013, 12:58 a.m., Mike Percy wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 370
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line370>
> >
> >     This is O(n) ... can we get it down to O(1) or O(log n)?

same as above.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16373
-----------------------------------------------------------
On Feb. 8, 2013, 11:05 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2013, 11:05 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION

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

(Updated Feb. 9, 2013, 2:28 a.m.)
Review request for Flume.
Changes
-------

Improve the checkedOutClients.remove calls performance, by making it a hash set - had to add hashCode and equals to the ClientWrapper class.
Description
-------

Added thrift rpc client. Detailed description posted on jira.
This addresses bug FLUME-1896.
    https://issues.apache.org/jira/browse/FLUME-1896
Diffs (updated)
-----

  flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
  flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
  flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/pom.xml ab066d5
  flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
  flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
  flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
  flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
  flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
  flume-ng-sources/flume-scribe-source/pom.xml 588207d
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
  flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
  flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
  flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
  pom.xml faa67f9

Diff: https://reviews.apache.org/r/9284/diff/
Testing
-------

Added unit tests for new code.
Thanks,

Hari Shreedharan

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

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34886>

    Would a small amount of synchronization and a BlockingQueue make this code a little simpler?

flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java
<https://reviews.apache.org/r/9284/#comment34893>

    I feel like we could do this in a little simpler fashion with a Semaphore?
- Brock Noland
On Feb. 9, 2013, 2:28 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2013, 2:28 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
>   flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
>   flume-ng-sources/flume-scribe-source/pom.xml 588207d
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
>   flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
>   pom.xml faa67f9
>
> Diff: https://reviews.apache.org/r/9284/diff/
>
>
> Testing
> -------
>
> Added unit tests for new code.
>
>
> Thanks,
>
> Hari Shreedharan
>
>

> On Feb. 10, 2013, 9:11 p.m., Brock Noland wrote:
> >

Brock:

Thanks for the review. I had considered both the options you mentioned in the comments, the reason I didn't do either of those was that using a BlockingQueue and/or semaphore adds an additional lock/unlock cycle inside the data structure code. Since we already need to lock for creating the clients, this code makes one less lock/unlock cycle (during creates, and destroys - both places where the queue and the counts are modified).

As such, I prefer the thread synchronization being more explicit than having multiple lock-enabled data structures.
> On Feb. 10, 2013, 9:11 p.m., Brock Noland wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 337
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line337>
> >
> >     Would a small amount of synchronization and a BlockingQueue make this code a little simpler?

I don't see it becoming much simpler. I also prefer having the threading code being more explicit. A blocking queue does not really help, since we are lazily initializing the clients, so we need locking for that anyway.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16399
-----------------------------------------------------------
On Feb. 9, 2013, 2:28 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2013, 2:28 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
>   flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
>   flume-ng-sources/flume-scribe-source/pom.xml 588207d
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11

> On Feb. 10, 2013, 9:11 p.m., Brock Noland wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 337
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line337>
> >
> >     I feel like we could do this in a little simpler fashion with a Semaphore?
>
> Brock Noland wrote:
>     Hari,
>    
>     Right now the checkout method has 9 lines of code doing non initialization work. Could this be done in 4-5 lines with a semaphore? I feel that is simpler and achieves the same result.
>    
>     semaphore.acquire(1);
>     o = availableClients.poll();
>     if(o == null) {
>       o = createNew();
>       checkedOutClients.add(o);
>     }
>    
>     then on return
>    
>     availableClients.put(o);
>     checkedOutClients.remove(o);
>     semaphore.release(1);
>
> Hari Shreedharan wrote:
>     Brock,
>    
>     I initially worked with this logic. But the reason I did not do this was that using a semaphore would mean that availableClients and checkedOutClients be thread-safe data structure, like a ConcurrentList or BlockingQueue or something. This would mean 3 lock/unlock cycles per checkOut and checkIn - one inside the semaphore code, one in each of the queue/sets. I was trying to avoid this. Since the critical sections are relatively small - the lock/unlock cost actually is more when we have 3 locks. The only real advantage I see from this approach is that the RpcClient can be created without holding the lock, but this happens relatively rarely in the life of the program, so I wouldn't worry too much about it.
>    
>     Agreed that the code becomes more concise, but I don't think we really need 3 locks to handle this especially in 3 consecutive lines of otherwise inexpensive code.
>
> Brock Noland wrote:
>     Hi,
>    
>     I feel this is pre-mature optimization. In the mean time each thread will be doing IO which is tremendously slow in comparison I don't think the faster but less readable code will buy us much. With that said, I haven't proved that and I don't feel strongly enough on the manner to hold up the commit. I have just committed this.
>    
>     Brock
>
> Hari Shreedharan wrote:
>     I agree that slow I/O threads can make things slow, but I don't still think it makes sense to use 3 blocking data structures when we could actually get away with none. Locks are expensive - even if they are not I/O level expensive, we are still hitting performance, when we don't really need to.

Also, this code is actually the standard -
enter monitor:
    while(condition) wait()
..
..
enter monitor:
    signal()

idiom. I don't really think it is less readable either ;)
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16399
-----------------------------------------------------------
On Feb. 9, 2013, 2:28 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2013, 2:28 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d

> On Feb. 10, 2013, 9:11 p.m., Brock Noland wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 337
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line337>
> >
> >     I feel like we could do this in a little simpler fashion with a Semaphore?
>
> Brock Noland wrote:
>     Hari,
>    
>     Right now the checkout method has 9 lines of code doing non initialization work. Could this be done in 4-5 lines with a semaphore? I feel that is simpler and achieves the same result.
>    
>     semaphore.acquire(1);
>     o = availableClients.poll();
>     if(o == null) {
>       o = createNew();
>       checkedOutClients.add(o);
>     }
>    
>     then on return
>    
>     availableClients.put(o);
>     checkedOutClients.remove(o);
>     semaphore.release(1);
>
> Hari Shreedharan wrote:
>     Brock,
>    
>     I initially worked with this logic. But the reason I did not do this was that using a semaphore would mean that availableClients and checkedOutClients be thread-safe data structure, like a ConcurrentList or BlockingQueue or something. This would mean 3 lock/unlock cycles per checkOut and checkIn - one inside the semaphore code, one in each of the queue/sets. I was trying to avoid this. Since the critical sections are relatively small - the lock/unlock cost actually is more when we have 3 locks. The only real advantage I see from this approach is that the RpcClient can be created without holding the lock, but this happens relatively rarely in the life of the program, so I wouldn't worry too much about it.
>    
>     Agreed that the code becomes more concise, but I don't think we really need 3 locks to handle this especially in 3 consecutive lines of otherwise inexpensive code.
>
> Brock Noland wrote:
>     Hi,
>    
>     I feel this is pre-mature optimization. In the mean time each thread will be doing IO which is tremendously slow in comparison I don't think the faster but less readable code will buy us much. With that said, I haven't proved that and I don't feel strongly enough on the manner to hold up the commit. I have just committed this.
>    
>     Brock

I agree that slow I/O threads can make things slow, but I don't still think it makes sense to use 3 blocking data structures when we could actually get away with none. Locks are expensive - even if they are not I/O level expensive, we are still hitting performance, when we don't really need to.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16399
-----------------------------------------------------------
On Feb. 9, 2013, 2:28 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2013, 2:28 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45

> On Feb. 10, 2013, 9:11 p.m., Brock Noland wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 337
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line337>
> >
> >     Would a small amount of synchronization and a BlockingQueue make this code a little simpler?
>
> Hari Shreedharan wrote:
>     I don't see it becoming much simpler. I also prefer having the threading code being more explicit. A blocking queue does not really help, since we are lazily initializing the clients, so we need locking for that anyway.

Sorry I thought I had replaced this comment with the one below.
> On Feb. 10, 2013, 9:11 p.m., Brock Noland wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 337
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line337>
> >
> >     I feel like we could do this in a little simpler fashion with a Semaphore?

Hari,

Right now the checkout method has 9 lines of code doing non initialization work. Could this be done in 4-5 lines with a semaphore? I feel that is simpler and achieves the same result.

semaphore.acquire(1);
o = availableClients.poll();
if(o == null) {
  o = createNew();
  checkedOutClients.add(o);
}

then on return

availableClients.put(o);
checkedOutClients.remove(o);
semaphore.release(1);
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16399
-----------------------------------------------------------
On Feb. 9, 2013, 2:28 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2013, 2:28 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
>   flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
>   flume-ng-sources/flume-scribe-source/pom.xml 588207d
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c

> On Feb. 10, 2013, 9:11 p.m., Brock Noland wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 337
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line337>
> >
> >     I feel like we could do this in a little simpler fashion with a Semaphore?
>
> Brock Noland wrote:
>     Hari,
>    
>     Right now the checkout method has 9 lines of code doing non initialization work. Could this be done in 4-5 lines with a semaphore? I feel that is simpler and achieves the same result.
>    
>     semaphore.acquire(1);
>     o = availableClients.poll();
>     if(o == null) {
>       o = createNew();
>       checkedOutClients.add(o);
>     }
>    
>     then on return
>    
>     availableClients.put(o);
>     checkedOutClients.remove(o);
>     semaphore.release(1);

Brock,

I initially worked with this logic. But the reason I did not do this was that using a semaphore would mean that availableClients and checkedOutClients be thread-safe data structure, like a ConcurrentList or BlockingQueue or something. This would mean 3 lock/unlock cycles per checkOut and checkIn - one inside the semaphore code, one in each of the queue/sets. I was trying to avoid this. Since the critical sections are relatively small - the lock/unlock cost actually is more when we have 3 locks. The only real advantage I see from this approach is that the RpcClient can be created without holding the lock, but this happens relatively rarely in the life of the program, so I wouldn't worry too much about it.

Agreed that the code becomes more concise, but I don't think we really need 3 locks to handle this especially in 3 consecutive lines of otherwise inexpensive code.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16399
-----------------------------------------------------------
On Feb. 9, 2013, 2:28 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2013, 2:28 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
>   flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION

> On Feb. 10, 2013, 9:11 p.m., Brock Noland wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java, line 337
> > <https://reviews.apache.org/r/9284/diff/10/?file=257203#file257203line337>
> >
> >     I feel like we could do this in a little simpler fashion with a Semaphore?
>
> Brock Noland wrote:
>     Hari,
>    
>     Right now the checkout method has 9 lines of code doing non initialization work. Could this be done in 4-5 lines with a semaphore? I feel that is simpler and achieves the same result.
>    
>     semaphore.acquire(1);
>     o = availableClients.poll();
>     if(o == null) {
>       o = createNew();
>       checkedOutClients.add(o);
>     }
>    
>     then on return
>    
>     availableClients.put(o);
>     checkedOutClients.remove(o);
>     semaphore.release(1);
>
> Hari Shreedharan wrote:
>     Brock,
>    
>     I initially worked with this logic. But the reason I did not do this was that using a semaphore would mean that availableClients and checkedOutClients be thread-safe data structure, like a ConcurrentList or BlockingQueue or something. This would mean 3 lock/unlock cycles per checkOut and checkIn - one inside the semaphore code, one in each of the queue/sets. I was trying to avoid this. Since the critical sections are relatively small - the lock/unlock cost actually is more when we have 3 locks. The only real advantage I see from this approach is that the RpcClient can be created without holding the lock, but this happens relatively rarely in the life of the program, so I wouldn't worry too much about it.
>    
>     Agreed that the code becomes more concise, but I don't think we really need 3 locks to handle this especially in 3 consecutive lines of otherwise inexpensive code.

Hi,

I feel this is pre-mature optimization. In the mean time each thread will be doing IO which is tremendously slow in comparison I don't think the faster but less readable code will buy us much. With that said, I haven't proved that and I don't feel strongly enough on the manner to hold up the commit. I have just committed this.

Brock
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16399
-----------------------------------------------------------
On Feb. 9, 2013, 2:28 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2013, 2:28 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION

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

Patch looks good, I tried running tests and got the following which does not occur on naked trunk:

[INFO] Scanning for projects...
[ERROR] The build could not read 1 project -> [Help 1]
[ERROR]  
[ERROR]   The project org.apache.flume:flume-ng-core:1.4.0-SNAPSHOT (/Users/noland/workspaces/flume-apache-trunk-golden/flume/flume-ng-core/pom.xml) has 1 error
[ERROR]     'dependencies.dependency.version' for org.apache.flume:flume-ng-sdk:jar:tests is missing. @ line 140, column 17
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/ProjectBuildingException
- Brock Noland
On Feb. 9, 2013, 2:28 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2013, 2:28 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
>   flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
>   flume-ng-sources/flume-scribe-source/pom.xml 588207d
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
>   flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
>   pom.xml faa67f9
>
> Diff: https://reviews.apache.org/r/9284/diff/

> On Feb. 11, 2013, 4:14 p.m., Brock Noland wrote:
> > Hi,
> >
> > Patch looks good, I tried running tests and got the following which does not occur on naked trunk:
> >
> > [INFO] Scanning for projects...
> > [ERROR] The build could not read 1 project -> [Help 1]
> > [ERROR]  
> > [ERROR]   The project org.apache.flume:flume-ng-core:1.4.0-SNAPSHOT (/Users/noland/workspaces/flume-apache-trunk-golden/flume/flume-ng-core/pom.xml) has 1 error
> > [ERROR]     'dependencies.dependency.version' for org.apache.flume:flume-ng-sdk:jar:tests is missing. @ line 140, column 17
> > [ERROR]
> > [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
> > [ERROR] Re-run Maven using the -X switch to enable full debug logging.
> > [ERROR]
> > [ERROR] For more information about the errors and possible solutions, please read the following articles:
> > [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/ProjectBuildingException
> >

Please ignore, wrong JIRA.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9284/#review16420
-----------------------------------------------------------
On Feb. 9, 2013, 2:28 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2013, 2:28 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
>   flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
>   flume-ng-sources/flume-scribe-source/pom.xml 588207d
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
>   flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION

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

Ship it!
Ship It!

- Brock Noland
On Feb. 9, 2013, 2:28 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9284/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2013, 2:28 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Added thrift rpc client. Detailed description posted on jira.
>
>
> This addresses bug FLUME-1896.
>     https://issues.apache.org/jira/browse/FLUME-1896
>
>
> Diffs
> -----
>
>   flume-ng-legacy-sources/flume-thrift-source/pom.xml b9667cd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java 327107a
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java d2495d2
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java 2bb6cfd
>   flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java 0f2ad2d
>   flume-ng-legacy-sources/flume-thrift-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/pom.xml ab066d5
>   flume-ng-sdk/src/main/java/org/apache/flume/api/HostInfo.java 7388a45
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java ab4c3de
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java 3c93921
>   flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java PRE-CREATION
>   flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java PRE-CREATION
>   flume-ng-sdk/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sdk/src/main/thrift/flume.thrift PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestThriftRpcClient.java PRE-CREATION
>   flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java PRE-CREATION
>   flume-ng-sources/flume-scribe-source/pom.xml 588207d
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java 401a7e4
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java 6bfa84c
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java ccde51b
>   flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java 6190b11
>   flume-ng-sources/flume-scribe-source/src/main/thrift/aslv2 PRE-CREATION
>   flume-ng-sources/flume-scribe-source/src/main/thrift/scribe-source.thrift PRE-CREATION
>   pom.xml faa67f9
>
> Diff: https://reviews.apache.org/r/9284/diff/
>
>
> Testing
> -------
>
> Added unit tests for new code.
>
>
> Thanks,
>
> Hari Shreedharan
>
>