>>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 >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>> >>>
