FLUME-3262: Improve Scribe Source With More Options

[JohnZZGithub]

We added following options:

1. ThriftServerType, user choose one of {THshaServer, TThreadedSelectorServer, TThreadPoolServer} baed on their own traffic pattern. More details on thrift server could be found on Apache Thrift project.
2. MaxThriftFrameSizeBytes: thrift frame size limit.
3. MaxReadBufferSize. The buffer size thrift used to handle requests, increase the buffer will benefit throughput under heavy load.

[asfgit]

Can one of the admins verify this patch?

[szaboferee]

@JohnZZGithub thanks for the contribution!

It is a really good feature to have more options.
To make the new feature more accessible for others, could you add the parameters to the documentation with some examples?

It would also be great if you could include some test cases that make sure that the new code works.

We could extract the creation logic into a factory to maintain readability.

[JohnZZGithub]

The failure seems not to be caused by patch https://builds.apache.org/job/flume-github-pull-request/org.apache.flume$flume-ng-tests/82/testReport/junit/org.apache.flume.test.agent/TestConfig/testConfigReplacement/

[majorendre]

I would include the bad value in the error message, to help finding the bad config.
Could we throw an exception here instead of the error logging?

[JohnZZGithub]

@majorendre Hi, I updated the patch and added an IllegalArgumentException here. However, the caller which is @Override public void configure(Context context) can't throw any exception as it overrides an non-exception method.

[majorendre]

Hi @JohnZZGithub
Thanks for the quick reply.
I took a deeper look and I suggest not handling that IllegalArgumentException at all. You can just let it propagate, Flume will handle it properly. I did test this, to be sure.
See my attached diff. I also added the Apache licence to avoid 'rat' errors.
As @szaboferee mentioned before, please add some test cases.
Regards,
Endre

[majorendre]

scribe-errhand.diff.zip
@JohnZZGithub

[turcsanyip]

Hi @JohnZZGithub, thanks for the contribution.
The concept looks fine, but please add tests and documentation as it was mentioned by other reviewers.

I tried to set up Scribe source with different Thrift server types and feed the source with the CLI tool from #218.
ThshaServer and TThreadedSelectorServer work fine for me, but I get the following error when I start the source in TThreadPoolServer mode:
2018-11-09 22:18:58,445 (Thread-2) [WARN - org.apache.thrift.server.TThreadPoolServer.serve(TThreadPoolServer.java:207)] Transport error occurred during acceptance of message. org.apache.thrift.transport.TTransportException: accept() may not return NULL at org.apache.thrift.transport.TServerTransport.accept(TServerTransport.java:62) ~[libthrift-0.9.3.jar:0.9.3] at org.apache.thrift.server.TThreadPoolServer.serve(TThreadPoolServer.java:162) [libthrift-0.9.3.jar:0.9.3] at org.apache.flume.source.scribe.ScribeSource$Startup.run(ScribeSource.java:78) [flume-scribe-source-1.9.0-SNAPSHOT.jar:1.9.0-SNAPSHOT]

[JohnZZGithub]

@majorendre Thanks for the patch, will add more tests.
@turcsanyip Will test it and get back to you later.

[szaboferee]

@JohnZZGithub could you please rebase your change to the latest trunk? It cannot be applied at the moment.

[JohnZZGithub]

@szaboferee Sure, will probably update the patch today with more tests

[JohnZZGithub]

@turcsanyip The failure of TThreadPoolServer is called by wrong socket type (should be blocking server socket), fixed now. Thanks

[JohnZZGithub]

Hi, I had updated the patch, please review it when you get a chance, thank you!

[szaboferee]

Hi @JohnZZGithub the checks failed. I thought You did not have the time to fix it. I personally start the review after the build is green to save the extra rounds. Feel free to run a mvn clean verify locally to check if the build is successful before you commit.

[JohnZZGithub]

@szaboferee Ah, sorry, my bad. I had only clicked into the first link and it shows 500 erros. So I thought it was not my patch's problem by mistake. Now I fix it. It's a licences issue.

[JohnZZGithub]

@szaboferee @majorendre I updated the diff. Please review it again when you get a chance, thanks.

[JohnZZGithub]

@mpercy Could you please help to review the change? Thanks