> On Aug. 24, 2016, 4:30 p.m., Attila Simon wrote: > > w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java, > > lines 52-56 > > <https://reviews.apache.org/r/51244/diff/6/?file=1482094#file1482094line52> > > > > 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.
With only these 5 fields, I think this way makes it easier to navigate code. Is there any other benefit to moving them? > On Aug. 24, 2016, 4:30 p.m., Attila Simon wrote: > > w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java, > > lines 66-73 > > <https://reviews.apache.org/r/51244/diff/6/?file=1482094#file1482094line66> > > > > 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) According to http://stackoverflow.com/a/502252 , it's a *nice* thing, however not necessary. I can remove them, although I think it's fine if they remain. > On Aug. 24, 2016, 4:30 p.m., Attila Simon wrote: > > w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java, > > line 120 > > <https://reviews.apache.org/r/51244/diff/6/?file=1482094#file1482094line120> > > > > 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) LogRawDataUtil is unfortunately not yet in the codebase, however we can change this if required in the future. On trace level it could be a useful thing to have the event logged for now. > On Aug. 24, 2016, 4:30 p.m., Attila Simon wrote: > > w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java, > > lines 164-168 > > <https://reviews.apache.org/r/51244/diff/6/?file=1482094#file1482094line164> > > > > nit: LOG.debug("Creating RemoveHeaderInterceptor with: withName={}, > > fromList={}, " + > > "listSeparator={}, matchRegex={}", new String[]{ > > withName, fromList, listSeparator, matchRegex.toString()}) Fixed. > On Aug. 24, 2016, 4:30 p.m., Attila Simon wrote: > > w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java, > > line 139 > > <https://reviews.apache.org/r/51244/diff/6/?file=1482094#file1482094line139> > > > > 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. I think there is a benefit to keeping the event in the log, but I changed the code to collect the headers for logging If you think the whole event logging is a blocking issue, please re-open > On Aug. 24, 2016, 4:30 p.m., Attila Simon wrote: > > w/flume-ng-core/src/main/java/org/apache/flume/interceptor/RemoveHeaderInterceptor.java, > > lines 124-130 > > <https://reviews.apache.org/r/51244/diff/6/?file=1482094#file1482094line124> > > > > 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) Changed the implementation for fromList to HashSet for better performance - Balázs Donát ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51244/#review146665 ----------------------------------------------------------- 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 > >
