----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51244/#review146503 -----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (line 49) <https://reviews.apache.org/r/51244/#comment212999> `matching` missing flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (line 53) <https://reviews.apache.org/r/51244/#comment213005> This pattern (defining interfaces to hold constants only) is considered bad practice. In this particular case it'd be better to put all the constants to the outer class as package private members (maybe prefixed with `CONFIG_`). I don't know whether we have any general pattern in flume for this, but looking at the other interceptor implementations I don't think so. * https://books.google.hu/books?id=ka2VUBqHiWkC&lpg=PA98&ots=yZJgRiv5R2&pg=PA98#v=onepage&q&f=false flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (lines 63 - 65) <https://reviews.apache.org/r/51244/#comment213004> why `protected`? `private` should work too. flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (line 121) <https://reviews.apache.org/r/51244/#comment213006> Please keep an eye on FLUME-2954 as logging the event itself might be considered not safe according to that issue. flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (line 123) <https://reviews.apache.org/r/51244/#comment213000> Misleading comment: not *otherwise* but *also* flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (line 136) <https://reviews.apache.org/r/51244/#comment213003> If the current key was in the `fromList` and also matches the regex the subsequent `it.remove()` call will throw an `IllegalStateException`. Changing the `if` in line 133 to `else if` should solve this issue. flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (line 141) <https://reviews.apache.org/r/51244/#comment213007> Please keep an eye on FLUME-2954 as logging the event itself might be considered not safe according to that issue. - Denes Arvay On Aug. 22, 2016, 11:13 p.m., Balázs Donát Bessenyei wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51244/ > ----------------------------------------------------------- > > (Updated Aug. 22, 2016, 11:13 p.m.) > > > Review request for Flume. > > > Bugs: FLUME-2171 > https://issues.apache.org/jira/browse/FLUME-2171 > > > Repository: flume-git > > > Description > ------- > > I found Flume OG's decorators to handle event headers useful and some to be > missing from Flume NG. More specifically, we could have an interceptor to > remove headers from an event. > > > Diffs > ----- > > > flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/interceptor/RemoveHeaderInterceptorTest.java > PRE-CREATION > flume-ng-doc/sphinx/FlumeUserGuide.rst 5e677c6 > > Diff: https://reviews.apache.org/r/51244/diff/ > > > Testing > ------- > > All tests (besides the FLUME-2974-related ones) in flume-ng-core run > successfully > > I've used this config for manual testing: > > a1.sources = r1 > a1.sources.r1.type = netcat > a1.sources.r1.bind = 0.0.0.0 > a1.sources.r1.port = 6666 > a1.sources.r1.channels = c1 > > a1.channels = c1 > a1.channels.c1.type = memory > a1.channels.c1.capacity = 10000 > a1.channels.c1.transactionCapacity = 10000 > a1.channels.c1.byteCapacityBufferPercentage = 20 > a1.channels.c1.byteCapacity = 800000 > > a1.channels = c1 > a1.sinks = k1 > a1.sinks.k1.type = logger > a1.sinks.k1.channel = c1 > > > a1.sources.r1.interceptors = i1 i2 i3 > a1.sources.r1.interceptors.i1.type = timestamp > > a1.sources.r1.interceptors.i2.type = host > a1.sources.r1.interceptors.i2.hostHeader = hostname > > a1.sources.r1.interceptors.i3.type = remove_header > a1.sources.r1.interceptors.i3.with.name = timestamp > > > Thanks, > > Balázs Donát Bessenyei > >
