FLUME-2050 Upgrade to Log4j 2 #181

rgoers · 2017-11-03T15:56:01Z

This PR removes the use of log4j 1.x from Flume.

szaboferee · 2017-11-03T17:22:20Z

flume-ng-sinks/flume-hive-sink/metastore_db/README_DO_NOT_TOUCH_FILES.txt

@@ -0,0 +1,9 @@
+


Hi @rgoers , thanks for the pull request
I am wondering how this metastore_db folder is necessary to upgrade the log4j version.

This is some leftover from a failed test run. Could you please remove it so we could review the real changes?

I didn't modify those files so I'm not sure how they got in the commit. Out of curiosity, is there a reason they aren't copied to the target directory if the tests are going to modify them? Maven convention is that only items in the target directory should be modified by the build so I don't have my gitignore set up to skip these files.

I will revert those files as soon as I can.

rgoers · 2017-11-04T19:28:17Z

The extra files have been removed.

szaboferee · 2017-11-07T13:31:48Z

@rgoers
As the first test, I have started a simple test agent with the trunk and with the patch and I see some difference between the log format
before:
07 nov. 2017 14:25:00,226 INFO [conf-file-poller-0] (org.apache.flume.conf.FlumeConfiguration$AgentConfiguration.addProperty:930) - Added sinks: k1 Agent: a1

after:
07 Nov 2017 14:25:06,189 INFO [conf-file-poller-0] (org.apache.flume.conf.FlumeConfiguration$AgentConfiguration.addProperty:930) [] - Added sinks: k1 Agent: a1

I believe this upgrade should not introduce any change in the default log format.

rgoers · 2017-11-07T19:09:14Z

The format strings are exactly the same. The difference seems to be how %x is handled. In Log4j 1.x the NDC seems to be returned as null if there is nothing, so I am not sure why it is an empty string. In Log4j 2 the ThreadContext Stack returns an empty list which I believe is why you see "[]". I am curious - Does Flume even use the NDC? Very view applications use the NDC - most use the MDC/ThreadContext Map. A grep of Flume finds the only occurrence of NDC is in the Log4jAppender and that reference only retrieves the NDC from the logging event.

Log4j 2 has a way to handle this so I will give it a try and update the PR when I confirm it is working.

rgoers · 2017-11-08T06:43:24Z

@szaboferee I ran the agent in flume-ng-dist and the results I get don't match yours. With log4j 1.x I got
07 Nov 2017 23:28:57,156 INFO [lifecycleSupervisor-1-0] (org.apache.flume.node.PollingPropertiesFileConfigurationProvider.start:62) - Configuration provider starting
This doesn't match what you have above. For some reason you have the month as "nov.". I am not sure how you are getting that as the pattern uses "MMM" which is handled by SimpleDateFormat in Log4j 1, which prints Nov as I have here.

Also, based on what you posted above I modified the log4j 2 configuration so that it doesn't print anything if there is no NDC and there is only one space as that is what I see above. However, when I look at the output I get from Log4j 1 I am seeing 2 spaces before the '-'. I noticed that when I include that string here that the double space is being converted to a single space, so I am guessing I really need to modify the pattern to emit 2 spaces when the NDC isn't present instead of just 1.

Are you using the standard conf/log4j.properties in the gzipped tar built by flume-ng-dist? If you are I don't know how you are getting "nov.".

szaboferee · 2017-11-13T14:26:48Z

conf/log4j.properties

@@ -1,68 +0,0 @@
-#


I was wondering why were you changing log4j.properties to XML.
Are there benefits I am not aware of?

There might be systems where flume was integrated with log4j.properties format. What is the precedence if there are both .properties and .xml?

Log4j 2 doesn't support the log4j 1 properties format. Even if the log4j 2 properties format is used it is still different than the log4j 1 format. I prefer the XML format.

What happens if there is a log4j.properties (log4j 2 format) and a log4j.xml file on the classpath at the same time?

Log4j uses an @order annotation on the ConfigurationFactories to determine which factories get precedence. Properties have the highest, followed by yaml and then XML. So if a properties file is present it will be used instead of the xml file.

Great, thanks for the information.

szaboferee · 2017-11-13T14:29:35Z

flume-ng-clients/flume-ng-log4jappender/pom.xml

@@ -67,6 +67,7 @@ limitations under the License.
    <dependency>
      <groupId>log4j</groupId>
      <artifactId>log4j</artifactId>
+      <version>1.2.17</version>


Wouldn't be better to have this in the parent dependencyManagement as there are transitive dependencies that are still using log4j 1.x?

No. All references to log4j 1 have been removed except for this one instance.

All references in flume however, some dependencies of flume has it as a transitive dependency.
For example hive-shmis, hadoop-common, kafka_2.10.

We should harmonize the versions for these dependencies as well to avoid version mismatches.

I went through and excluded all the transitive dependencies to log4j in the Flume pom.xml files that I could find. They have been replaced with references to log4j-1.2-api.

Seeing the output of mvn dependency:tree there are a few places where it looks like you might have missed it. Could you please double check it to see if I am right?

Wouldn't it be easier to use the dependencyManagement in parent pom to exclude the old version?

It might. I will take a look again.

szaboferee · 2017-11-13T14:36:30Z

flume-ng-sinks/flume-http-sink/src/main/java/org/apache/flume/sink/http/HttpSink.java

@@ -27,7 +27,8 @@
 import org.apache.flume.conf.Configurable;
 import org.apache.flume.instrumentation.SinkCounter;
 import org.apache.flume.sink.AbstractSink;
-import org.apache.log4j.Logger;
+import org.apache.logging.log4j.LogManager;


I would prefer the slf4j logger here because that way the user could decide to use a different logger implementation.
(despite the fact that the original code already used log4j)

Because it was using log4j I converted it to use log4j...

Flume components are using slf4j. I guess this log4j logger in the HttpSink was overlooked at the review and that is why it is not an slf4j logger. It would be nice if it could be consistent.

I have no problem if that change is made but it doesn't necessarily have to happen as part of this PR.

I agree. I will make a different PR about it. Thank you.

rgoers · 2017-11-15T13:57:05Z

I have updated the PR again but it can't be merged. It requires Log4j 2.10, which hasn't been released yet, to resolve a ClassNotFound error in Hadoop. The release process for Log4j 2.10 will be starting in the next few days. I will update this PR when it has completed.

rgoers · 2017-11-24T07:43:37Z

The patch has been upgraded to use Log4j 2.10.0. That version includes a version of AppenderSkeleton from Log4j 1 which should eliminate problems with Hadoop. From what I can tell this PR should be ready to go now.

szaboferee

I checked the dependency tree, the only log4j 1.x is in the appender, tests ran successfully.

The change looks good to me.

This closes apache#181 Reviewers: Ferenc Szabo (Ralph Goers via Denes Arvay)

This closes apache#181 Reviewers: Ferenc Szabo (Ralph Goers via Denes Arvay) Change-Id: Ifd2175c384f1456d082a054af2b042c7147ed446

pili-stream_oqos(1.0.0):add spark streaming task for the ola of inter…

FLUME-2050 Upgrade to Log4j 2

140ea5d

szaboferee suggested changes Nov 3, 2017

View reviewed changes

FLUME-2050 remove spurious files

4a07fbf

FLUME-2050 - modify pattern layout so NDC is ignored if it has no data

4bb5e88

Update configuration to match log4j 1.x

6b6605c

szaboferee reviewed Nov 13, 2017

View reviewed changes

Remove more references to log4j 1

e4fd6ab

Upgrade to Log4j 2.10.0

6813d9c

szaboferee approved these changes Nov 24, 2017

View reviewed changes

asfgit merged commit 6813d9c into trunk Nov 24, 2017

adenes pushed a commit to adenes/flume that referenced this pull request Jan 26, 2018

FLUME-2050. Upgrade to Log4j 2.10.0

69c66ef

This closes apache#181 Reviewers: Ferenc Szabo (Ralph Goers via Denes Arvay)

szaboferee pushed a commit to szaboferee/flume that referenced this pull request Jan 16, 2019

FLUME-2050. Upgrade to Log4j 2.10.0

c7bd30a

This closes apache#181 Reviewers: Ferenc Szabo (Ralph Goers via Denes Arvay) Change-Id: Ifd2175c384f1456d082a054af2b042c7147ed446

waidr pushed a commit to waidr/flume that referenced this pull request Jul 24, 2019

Merge pull request apache#181 from waidr/stream-diagnostics-v3

b9461bd

pili-stream_oqos(1.0.0):add spark streaming task for the ola of inter…

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FLUME-2050 Upgrade to Log4j 2 #181

FLUME-2050 Upgrade to Log4j 2 #181

rgoers commented Nov 3, 2017

szaboferee Nov 3, 2017

szaboferee Nov 3, 2017

rgoers Nov 3, 2017

rgoers Nov 3, 2017

rgoers commented Nov 4, 2017

szaboferee commented Nov 7, 2017

rgoers commented Nov 7, 2017 •

edited

Loading

rgoers commented Nov 8, 2017

szaboferee Nov 13, 2017

rgoers Nov 13, 2017

szaboferee Nov 13, 2017

rgoers Nov 13, 2017

szaboferee Nov 13, 2017

szaboferee Nov 13, 2017

rgoers Nov 13, 2017

szaboferee Nov 13, 2017

rgoers Nov 13, 2017

szaboferee Nov 13, 2017

rgoers Nov 13, 2017

szaboferee Nov 13, 2017

rgoers Nov 13, 2017

szaboferee Nov 13, 2017

rgoers Nov 13, 2017

szaboferee Nov 13, 2017

rgoers commented Nov 15, 2017

rgoers commented Nov 24, 2017

szaboferee left a comment

FLUME-2050 Upgrade to Log4j 2 #181

FLUME-2050 Upgrade to Log4j 2 #181

Conversation

rgoers commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgoers commented Nov 4, 2017

szaboferee commented Nov 7, 2017

rgoers commented Nov 7, 2017 • edited Loading

rgoers commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgoers commented Nov 15, 2017

rgoers commented Nov 24, 2017

szaboferee left a comment

Choose a reason for hiding this comment

rgoers commented Nov 7, 2017 •

edited

Loading