> On 2012-06-02 21:11:53, Hari Shreedharan wrote: > > trunk/flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java, > > lines 215-216 > > <https://reviews.apache.org/r/5333/diff/1/?file=111427#file111427line215> > > > > You should be setting the batchSize of the localClient also. If this is > > not set the batchSize is never passed to the actual client that does the > > Rpc writes. > > Hari Shreedharan wrote: > The underlying Rpc client will send multiple batches of size=batchSize, > if the list has more events than batchSize. So the client need not check the > batchSize during append. It can pass list of arbitrary size, and the client > will send multiple batches till the list is empty. > > Mubarak Seyed wrote: > Thanks Hari. I can remove the check from the app client code but if user > specifies the custom batch-size then it always defaults to DEFAULT_BATCH_SIZE > by NettyAvroRpcClient. > > This patch will not fix if client app specifies the batch-size in > Properties (using RpcClientFactory.getInstance(Properties)) > > We need to send the batch-size all the way to > RpcClientFactory.getDefaultInstance(hostName, port, batchSize) from > getNextClient > > Today, batchSize is default to 0 which sets as DEFAULT_BATCH_SIZE (100) > > Will update the patch. Thanks.
That is correct. the batchSize needs to be passed through the factory.getDefaultInstance method. It seems like there is no setBatchSize method. Since batchSize is protected you can also directly do localClient.batchSize = this.batchSize (but passing it through the factory is better - I am not really a fan of accessing members directly from other classes). Basically: * Read the batch-size from properties to this.batchSize * Either call RpcClientFactory.getDefaultInstance(<hostName>, <port>, this.batchSize) or let the current getDefaultInstance code be the same and do localClient.batchSize = this.batchSize before calling appendBatch() [I prefer 1 over 2] - Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5333/#review8300 ----------------------------------------------------------- On 2012-06-02 18:12:37, Mubarak Seyed wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5333/ > ----------------------------------------------------------- > > (Updated 2012-06-02 18:12:37) > > > Review request for Flume. > > > Summary > ------- > > FailoverRpcClient uses batchSize as local variable and it breaks the contract > for getBatchSize(). Consumer of FailoverRpcClient checks for getBatchSize() > to send events using appendBatch(List<Event>), since batchSize is initialized > with null, clients gets NPE. > > The fix is to remove the batchSize local variable in configureHosts() > > > This addresses bug FLUME-1226. > https://issues.apache.org/jira/browse/FLUME-1226 > > > Diffs > ----- > > > trunk/flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java > 1342144 > > Diff: https://reviews.apache.org/r/5333/diff > > > Testing > ------- > > Tested in lab environment. > > > Thanks, > > Mubarak > >
