----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51244/#review146665 -----------------------------------------------------------
c/flume-ng-doc/sphinx/FlumeUserGuide.rst (lines 3474 - 3477) <https://reviews.apache.org/r/51244/#comment213222> Were there any particular reason why these aren't named like: withName matching fromList fromListSeparator w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (lines 52 - 56) <https://reviews.apache.org/r/51244/#comment213223> Have you considered moving these out to a RemoveHeaderInterceptorConstants final class (not interface as pointed out earlier)? Additional javadoc to them wouldn't uglify this class then. w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (lines 66 - 73) <https://reviews.apache.org/r/51244/#comment213237> nit: there is not anon-inner class involved here so final keyword usage on function arguments is not encouraged. (I have no hard opinion on this but received this feedback earlier) w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (line 120) <https://reviews.apache.org/r/51244/#comment213231> I know this has been already pointed out, I would vote for not printing out the event only the removed headers. (Or protect with LogRawDataUtil) w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (lines 124 - 130) <https://reviews.apache.org/r/51244/#comment213238> Iterating through the whole header keyset is inevitable if matching regexp was required. But if hasn't been set then this implementation looses the benefit coming from HashMap O(1) lookup complexity. Instead of iterating over the fromList and for each item headermap is checked, it iterates over each key from the map and then check fromList whether it contains it (fromList is backed by a sorted array as pointed out by javadocs of com.google.common.collect.ImmutableSortedSet which has poorer lookup performance than HashMap) w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (line 139) <https://reviews.apache.org/r/51244/#comment213233> Please don't log out the whole Event for each removed header. Also please collect all the headers which were removed and print them out as a single log entry per Event. w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java (lines 164 - 168) <https://reviews.apache.org/r/51244/#comment213235> nit: LOG.debug("Creating RemoveHeaderInterceptor with: withName={}, fromList={}, " + "listSeparator={}, matchRegex={}", new String[]{ withName, fromList, listSeparator, matchRegex.toString()}) - Attila Simon On Aug. 23, 2016, 2:43 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. 23, 2016, 2:43 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 > ----- > > > c/flume-ng-core/src/main/java/org/apache/flume/interceptor/InterceptorType.java > fe341e9 > c/flume-ng-doc/sphinx/FlumeUserGuide.rst 5e677c6 > > w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java > PRE-CREATION > > w/flume-ng-core/src/test/java/org/apache/flume/interceptor/RemoveHeaderInterceptorTest.java > PRE-CREATION > > 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 > >
