On Tue, Jan 11, 2022 at 8:20 AM 张铎(Duo Zhang) <[email protected]> wrote:
> So we could open an issue for this feature then? As we have already started > to discuss the implementation :) > Yes, that sounds like a good idea. Feel free to go ahead :-) Gary > > Thanks~ > > Volkan Yazıcı <[email protected]> 于2022年1月11日周二 19:42写道： > > > Giving it a one more try: > > > > <CompoundProperty> > > <Source>${sys:hadoop.root.logger}</Source> > > <Split delimiter="\s+" trim="true" blanksExcluded="true"> > > <item index="0">hadoop.root.logger.level</item> > > <slice startIndex="1">hadoop.root.logger.appender</slice> > > </Split> > > </CompoundProperty> > > > > On Mon, Jan 10, 2022 at 3:54 PM Ralph Goers <[email protected]> > > wrote: > > > > > Yeah, that syntax is better although it doesn’t solve the multiple > > > appender refs case either. > > > > > > LoggerConfig accepts a List<AppenderRef> so it would seem that if we > > > modify LoggerConfig > > > to add another attribute that is a string that creates an AppenderRef > for > > > each item then > > > one could do > > > > > > <Logger name=“xyzzy” level=“INFO” appenderRefNames=“a, b, c”/> > > > > > > > > > Ralph > > > > > > > On Jan 10, 2022, at 5:04 AM, Volkan Yazıcı <[email protected]> wrote: > > > > > > > > I am curious if shall we adopt a more generic approach to extraction: > > > > > > > > <CompoundProperty> > > > > <Source>${sys:hadoop.root.logger}</Source> > > > > <Targets pattern="^(.+)\s*,\s(.+)$"> > > > > <Target>hadoop.root.logger.level</Target> > > > > <Target>hadoop.root.logger.appender</Target> > > > > </Targets> > > > > </CompoundProperty> > > > > > > > > On Mon, Jan 10, 2022 at 5:53 AM Ralph Goers < > > [email protected]> > > > > wrote: > > > > > > > >> OK, I had a suspicion the reason for needing this was something like > > > that. > > > >> > > > >> The syntax you propose below would require modifying PropertiesUtil, > > the > > > >> Property > > > >> plugin and StrSubstitutor to support arrays. I don’t think I’d want > to > > > go > > > >> down that path. > > > >> StrSubstitutor is already complicated and the idea of enhancing it > to > > > >> support stuff like > > > >> this doesn’t excite me. > > > >> > > > >> I think we’d be better off creating a new plugin for this. Something > > > like: > > > >> > > > >> <CompoundProperty name=“hadoop.root.logger.level” split=“,” > > > >> index=“0”>${sys:hadoop.root.logger}</CompoundProperty> > > > >> <CompoundProperty name=“hadoop.root.logger.appender” split=“,” > > > >> index=“1”>${sys:hadoop.root.logger}</CompoundProperty> > > > >> > > > >> The only problem here is if you want to provide more than one > appender > > > >> this won’t > > > >> work well. But I am sure we could enhance AppenderRef to accept a > list > > > of > > > >> appenders. > > > >> > > > >> Ralph > > > >> > > > >> > > > >> > > > >> > > > >>> On Jan 9, 2022, at 5:29 PM, 张铎(Duo Zhang) <[email protected]> > > > wrote: > > > >>> > > > >>> Thank you for the reply. > > > >>> > > > >>> And yes, it is not only for the root logger, we have tons of > > different > > > >>> loggers in hadoop, as hadoop is constructed with several different > > > >> projects > > > >>> actually, and they are developed by different groups of people... > > > >>> > > > >>> And on splitting the config in shell, actually this is exactly > what I > > > >> have > > > >>> done in HBase > > > >>> > > > >>> See > > > >>> > > > >> > > > > > > https://github.com/apache/hbase/blob/3ec3df5887e9271f7e75779eafe2439012cfb2c3/bin/hbase#L829 > > > >>> > > > >>> And also > > > >>> > > > >> > > > > > > https://github.com/apache/hbase/blob/3ec3df5887e9271f7e75779eafe2439012cfb2c3/bin/hbase.cmd#L336 > > > >>> > > > >>> For HBase maybe it is acceptable but hadoop is another story. > > > >>> Hadoop is widely used almost in every bigdata platform. Among the > > > >>> companies I've worked with, they all have their own hadoop > deployment > > > >>> systems which have modified version of start up scripts... > > > >>> > > > >>> I can imagine that if we do the same trick in HBase, then people > will > > > ask > > > >>> again and again on the mailing list why passing > -Dhadoop.root.logger > > > does > > > >>> not work, as well as other options like -Dyarn.root.logger, > > > >>> -Dhadoop.security.logger, etc... > > > >>> > > > >>> So it will be good if we can support configure level and appenders > in > > > one > > > >>> property. > > > >>> > > > >>> Or maybe another way is to enhance the ability in the Properties > > > section, > > > >>> so we can split one property into two properties, something like > > > >>> > > > >>> <Properties> > > > >>> <Property name="hadoop.root.logger" > > > >>> type="array">${sys:hadoop.root.logger:-INFO,Console}</Property> > > > >>> <Property > > > >>> name="hadoop.root.logger.level">${hadoop.root.logger[0]}</Property> > > > >>> <Property name="hadoop.root.logger.appender"> > > ${hadoop.root.logger[1]} > > > >>> </Property> > > > >>> <Properties> > > > >>> > > > >>> Notice that since we could add multiple appenders here, maybe we > need > > > to > > > >>> support something like ${hadoop.root.logger[1:] to combine all the > > > >>> appenders... > > > >>> This could also solve the problem as people could still pass > > > >>> -Dhadoop.root.logger. > > > >>> > > > >>> But I'm not sure if adding the ability to split a string in > > > configuation > > > >>> could introduce new possible security concerns... > > > >>> > > > >>> Thanks. > > > >>> > > > >>> Ralph Goers <[email protected]> 于2022年1月10日周一 07:14写道： > > > >>> > > > >>>> We already support this with two variables. But if they only want > to > > > >> pass > > > >>>> one then we have two options: > > > >>>> 1. Modify PropertiesConfiguration to support a new element that > > > allows a > > > >>>> Level and AppenderRef which then gets split internally. > > > >>>> 2. Create a Lookup that extracts the relevant portion of the data. > > > >>>> > > > >>>> Ralph > > > >>>> > > > >>>>> On Jan 9, 2022, at 3:08 PM, Gary Gregory <[email protected] > > > > > >> wrote: > > > >>>>> > > > >>>>> I think it is reasonable to say we can support this through 2 > > instead > > > >> of > > > >>>> 1 > > > >>>>> variable. > > > >>>>> > > > >>>>> Duo? > > > >>>>> > > > >>>>> Gary > > > >>>>> > > > >>>>> On Sun, Jan 9, 2022, 16:24 Ralph Goers < > [email protected] > > > > > > >>>> wrote: > > > >>>>> > > > >>>>>> I’m looking at this and have a couple of concerns. The script > has > > > >>>>>> > > > >>>>>> > > > >>>>>> > > HADOOP_ROOT_LOGGER=${HADOOP_ROOT_LOGGER:-${HADOOP_LOGLEVEL},console} > > > >>>>>> > > > >>>>>> > > > >>>> > > > >> > > > > > > HADOOP_DAEMON_ROOT_LOGGER=${HADOOP_DAEMON_ROOT_LOGGER:-${HADOOP_LOGLEVEL},RFA} > > > >>>>>> > > HADOOP_SECURITY_LOGGER=${HADOOP_SECURITY_LOGGER:-INFO,NullAppender} > > > >>>>>> > > > >>>>>> So it seems you need this for more than just the root logger. > > > >>>>>> > > > >>>>>> Second, you are asking us to accept “level, appender” as the > > value > > > >> and > > > >>>>>> under the covers split the > > > >>>>>> value and assign the level to the level attribute and the > appender > > > >> name > > > >>>> to > > > >>>>>> the AppenderRef. > > > >>>>>> > > > >>>>>> While this certainly can be done it seems like it would be just > as > > > >> easy > > > >>>> to > > > >>>>>> do the split in the script > > > >>>>>> and create two different variables for the logging configuration > > to > > > >> pick > > > >>>>>> up. Is there a reason that > > > >>>>>> you can’t do this - for example perhaps users have hacked your > > > scripts > > > >>>> and > > > >>>>>> now you can’t > > > >>>>>> change them without breaking them? If so, was this “supported”? > > > >>>>>> > > > >>>>>> Ralph > > > >>>>>> > > > >>>>>> > > > >>>>>> > > > >>>>>>> On Jan 9, 2022, at 8:01 AM, 张铎(Duo Zhang) < > [email protected] > > > > > > >>>> wrote: > > > >>>>>>> > > > >>>>>>> I brought this up in the incubator mailing list, and was > > suggested > > > to > > > >>>>>>> report directly to the log4j community. > > > >>>>>>> > > > >>>>>>> https://issues.apache.org/jira/browse/HADOOP-16206 > > > >>>>>>> > > > >>>>>>> In hadoop we started to try migrating to log4j2 long ago, but > it > > is > > > >> not > > > >>>>>>> easy. For now, one of the most blocker issues is the lack of > > > support > > > >> of > > > >>>>>>> 'log4j.rootLogger=INFO,Console' grammar. Notice that we do not > > want > > > >> to > > > >>>>>> stay > > > >>>>>>> on the bridge api, we want to fully migrate to log4j2 finally, > so > > > >>>>>>> supporting the above grammar in log4j1 bridge is not enough. > > > >>>>>>> > > > >>>>>>> This is the main log4j configuration file for hadoop > > > >>>>>>> > > > >>>>>>> > > > >>>>>> > > > >>>> > > > >> > > > > > > https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/conf/log4j.properties > > > >>>>>>> > > > >>>>>>> We have this in the file > > > >>>>>>> > > > >>>>>>> # Define the root logger to the system property > > > "hadoop.root.logger". > > > >>>>>>>> log4j.rootLogger=${hadoop.root.logger} > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> So our users could simply pass -Dhadoop.root.logger to control > > the > > > >>>> level > > > >>>>>>> and appender. > > > >>>>>>> > > > >>>>>>> > > > >>>>>> > > > >>>> > > > >> > > > > > > https://github.com/apache/hadoop/blob/39efbc6b6fe53abed15f5639edcbaaa2a9dda6d2/hadoop-common-project/hadoop-common/src/main/bin/hadoop-functions.sh#L904 > > > >>>>>>> > > > >>>>>>> Here we use an environment variable in our start up scripts. > And > > I > > > >>>>>> believe > > > >>>>>>> there are lots of other hadoop deployment systems which will > have > > > >> their > > > >>>>>> own > > > >>>>>>> start scripts. > > > >>>>>>> > > > >>>>>>> So in general, it is just impossible for us to drop the > > > >>>>>>> -Dhadoop.root.logger way of configuring our logging system. > > > >>>>>>> > > > >>>>>>> So here I want to ask if it is possible for log4j2 to still > > support > > > >> the > > > >>>>>>> 'log4j.rootLogger=INFO,Console' grammar. I'm not saying you > must > > > add > > > >> it > > > >>>>>>> back with no change, we just need a way to configure the level > > and > > > >>>>>> appeners > > > >>>>>>> at once, so something like > > > >>>>>>> 'log4j2.rootLogger.levelAndAppender=INFO,Console' is also OK. > > > >>>>>>> > > > >>>>>>> Thanks. > > > >>>>>> > > > >>>>>> > > > >>>> > > > >>>> > > > >> > > > >> > > > > > > > > >
