Hi Mohammad, If you want to file a JIRA with a patch for the partial revert, then please go ahead. Feel free to notify me on the issue for code review. Thanks!
--Chris Nauroth On 3/17/16, 12:23 AM, "Mohammad arshad" <[email protected]> wrote: >>>1. Full revert of ZOOKEEPER-1371, targeted to 3.5.2. >Can we do partial revert instead of full > i) revert log4j and slf4j test scoped dependency to run time dependency, >as it we earlier before this patch was merged > ii) revert the changes done in build.xml >For partial revert we can create another path on top of latest code, I >can create the patch. >This will avoid lot of duplicate effort > >>>2. Retarget ZOOKEEPER-1371 to 3.5.3 with the scope ... >This is not required as if above is done > >>> 3. Retarget ZOOKEEPER-2342 to 3.6.0 ... >+1 for this > > >-----Original Message----- >From: Flavio Junqueira [mailto:[email protected]] >Sent: 17 March 2016 12:21 >To: [email protected] >Cc: Chris Nauroth >Subject: Re: We need to prioritize getting logging fixed in trunk/3.5 > >Agreed, sounds like a good plan, +1. > >Thanks, Chris! > >-Flavio > >> On 17 Mar 2016, at 06:02, Patrick Hunt <[email protected]> wrote: >> >> That makes sense to me as well. +1. Thank you Chris! >> >> Patrick >> >> On Wed, Mar 16, 2016 at 9:42 AM, Raúl Gutiérrez Segalés >><[email protected]> wrote: >>> +1. This sounds like a good plan. Thanks Chris! >>> On Mar 16, 2016 9:36 AM, "Chris Nauroth" <[email protected]> >>>wrote: >>> >>>> We now have multiple binding +1's for a revert. To finalize the >>>> plan, here is what I propose: >>>> >>>> 1. Full revert of ZOOKEEPER-1371, targeted to 3.5.2. >>>> >>>> 2. Retarget ZOOKEEPER-1371 to 3.5.3 with the scope limited to just >>>> the SLF4J logging API changes. We'd omit the build changes that >>>> dropped the SLF4J-Log4J 1.2 binding from the distro. This would be >>>> a backwards-compatible change, and I believe it was the original >>>> intent of ZOOKEEPER-1371. This is not critical to complete for >>>> 3.5.3. I'm just pushing it ahead to the next version. >>>> >>>> 3. Retarget ZOOKEEPER-2342 to 3.6.0 for tracking Log4J 2 migration. >>>> This will have to happen someday since Log4J 1 is end of life, but >>>> it will likely be backwards-incompatible, and the change provides no >>>> value add to justify it for the 3.5 line. >>>> >>>> I'll wait 24 hours before proceeding with a revert in case anyone >>>> else wants to comment. >>>> >>>> --Chris Nauroth >>>> >>>> >>>> >>>> >>>> On 3/16/16, 6:47 AM, "Camille Fournier" <[email protected]> wrote: >>>> >>>>> I'm a strong +1 to get this fixed even if it requires reverting the >>>>> original patch. Broken logging is huge. Let's do whatever is >>>>> expedient and sensible to fix it. >>>>> >>>>> On Tue, Mar 15, 2016 at 7:27 PM, Chris Nauroth >>>>> <[email protected]> >>>>> wrote: >>>>> >>>>>> Yes, that's basically my assessment too. Copy-pasting my earlier >>>>>> comment from ZOOKEEPER-1371: >>>>>> >>>>>> "After this patch, ZooKeeper no longer produces any logging, >>>>>> because there is no SLF4J binding jar available on the runtime >>>>>> classpath." >>>>>> >>>>>> >>>>>> There is no compatibility problem with switching to SLF4J >>>>>> exclusively as our API of choice for logging instead of calling >>>>>> the Log4J API. The incompatible part is that the distro isn't >>>>>> shipping with any SLF4J binding included. Perhaps we can do a >>>>>> partial revert of just that part of ZOOKEEPER-1371. >>>>>> >>>>>> --Chris Nauroth >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On 3/15/16, 11:54 AM, "Patrick Hunt" <[email protected]> wrote: >>>>>> >>>>>>> Hm, I started looking at the original patch in more depth: >>>>>>> >>>>>> >>>>>> >>>> https://issues.apache.org/jira/secure/attachment/12773684/ZOOKEEPER- >>>> 1371- >>>>>> 0 >>>>>>> 5.patch >>>>>>> >>>>>>> is the real root issue 2342 is trying to address the following >>>>>>> line >>>>>>> change: >>>>>>> >>>>>>> - <dependency org="org.slf4j" name="slf4j-log4j12" rev="1.7.5" >>>>>>> transitive="false"/> >>>>>>> + <dependency org="org.slf4j" name="slf4j-log4j12" rev="1.7.5" >>>>>>> transitive="false" conf="test->default"/> >>>>>>> >>>>>>> Specifically that we changed from runtime to test only for this >>>>>>> dependency? Perhaps we just need to revert that? I see some other >>>>>>> magic happening in the build.xml file that I don't quite >>>>>>> understand - adding a new target and NoLog4j... references. >>>>>>> >>>>>>> Raul perhaps you can give more insight since it seems like you >>>>>>> worked on 1371 most recently? >>>>>>> >>>>>>> Patrick >>>>>>> >>>>>>> >>>>>>> On Tue, Mar 15, 2016 at 11:41 AM, Chris Nauroth >>>>>>> <[email protected]> wrote: >>>>>>>> I agree. Even if we don't fully understand every minute >>>>>>>> technical detail of Log4J 2 vs. Log4J 1, I think we've learned >>>>>>>> enough from my work-in-progress patch to declare that a >>>>>>>> migration is too risky for >>>>>> the >>>>>>>> 3.5 line. Reverting ZOOKEEPER-1371 (the earlier >>>>>> backwards-incompatible >>>>>>>> logging change) is the better choice for the interest of >>>>>>>> proceeding >>>>>> with >>>>>>>> 3.5 releases. >>>>>>>> >>>>>>>> --Chris Nauroth >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 3/15/16, 11:23 AM, "Patrick Hunt" <[email protected]> wrote: >>>>>>>> >>>>>>>>> I just commented on ZOOKEEPER-2342... not sure I fully >>>>>>>>> understand all the issues to be honest. Given how much we're >>>>>>>>> trying to do in 3.5 it seems like it would be prudent to wait >>>>>>>>> on 1371 until 3.6... IMO. :-) >>>>>>>>> >>>>>>>>> Patrick >>>>>>>>> >>>>>>>>> On Tue, Mar 15, 2016 at 11:15 AM, Chris Nauroth >>>>>>>>> <[email protected]> wrote: >>>>>>>>>> At this point, I am +1 for a revert of the patch that >>>>>>>>>> introduced >>>>>> the >>>>>>>>>> problem (ZOOKEEPER-1371). We need more time to come up with a >>>>>>>>>> migration path to Log4J 2 that minimizes impact on operators. >>>>>>>>>> That will take time, and I'd prefer that we don't hold up >>>>>>>>>> 3.5.2-alpha for it. >>>>>>>>>> >>>>>>>>>> --Chris Nauroth >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 3/15/16, 11:08 AM, "Patrick Hunt" <[email protected]> wrote: >>>>>>>>>> >>>>>>>>>>> Hi folks, can we prioritize getting logging fixed? It's >>>>>>>>>>> causing >>>>>> test >>>>>>>>>>> failures, e.g.: >>>>>>>>>>> >>>>>> https://builds.apache.org/job/ZooKeeper-trunk/2850/artifact/trunk/ >>>>>> buil >>>>>>>>>>> d/ >>>>>>>>>>> tm >>>>>>>>>>> p/zk.log >>>>>>>>>>> >>>>>>>>>>> This is the jira: >>>>>>>>>>> https://issues.apache.org/jira/browse/ZOOKEEPER-2342 >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Perhaps we should revert the change that caused this in the >>>>>>>>>>> first place. >>>>>>>>>>> >>>>>>>>>>> Patrick >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>> >>>> >
