Solr
  1. Solr
  2. SOLR-8324 Logger Untanglement
  3. SOLR-8359

Restrict child classes from using parent logger's state

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      In SOLR-8330 we split up a lot of loggers. However, there are a few classes that still use their parent's logging state and configuration indirectly.

      HdfsUpdateLog and HdfsTransactionLog both use their parent class cached read of boolean debug = log.isDebugEnabled(), when they should check their own loggers instead.

      1. SOLR-8359.patch
        4 kB
        Yonik Seeley
      2. SOLR-8359.patch
        4 kB
        Jason Gerlowski
      3. SOLR-8359.patch
        3 kB
        Jason Gerlowski
      4. SOLR-8359-nonfinal-values.patch
        4 kB
        Jason Gerlowski

        Activity

        Hide
        Mike Drob added a comment -

        Jason Gerlowski - do you want to take a look at this? I didn't notice it missing when looking at the original patch, but it looks like a pretty straightforward fix.

        Show
        Mike Drob added a comment - Jason Gerlowski - do you want to take a look at this? I didn't notice it missing when looking at the original patch, but it looks like a pretty straightforward fix.
        Hide
        Mike Drob added a comment -

        PeerSync also has this, but no other classes use the cached debug value there.

        Show
        Mike Drob added a comment - PeerSync also has this, but no other classes use the cached debug value there.
        Hide
        Jason Gerlowski added a comment -

        Sure, good catch Mike, I didn't even think of this.

        I did a quick grep around for other instances of this. So add CdcrTransactionLog to the list. Hopefully we're not missing any other classes here.

        Should anything go in CHANGES.txt for this JIRA? I lean towards "no", since this is just fallout from another change that's already mentioned in CHANGES.txt. But I'm not very familiar with when/how/why things get added to that file, so maybe I've got the wrong approach.

        I'm running tests on a patch now, will upload it shortly if all goes well.

        Show
        Jason Gerlowski added a comment - Sure, good catch Mike, I didn't even think of this. I did a quick grep around for other instances of this. So add CdcrTransactionLog to the list. Hopefully we're not missing any other classes here. Should anything go in CHANGES.txt for this JIRA? I lean towards "no", since this is just fallout from another change that's already mentioned in CHANGES.txt. But I'm not very familiar with when/how/why things get added to that file, so maybe I've got the wrong approach. I'm running tests on a patch now, will upload it shortly if all goes well.
        Hide
        Jason Gerlowski added a comment -

        Tests passed; here's the patch.

        Show
        Jason Gerlowski added a comment - Tests passed; here's the patch.
        Hide
        Mike Drob added a comment -

        Not sure it makes sense to always make these cached boolean private static final. UpdateLog.preSoftCommit would refresh the values, I think HdfsUpdateLog now misses out on that, haven't confirmed though. Logging levels can change dynamically via the Solr UI or through file watchdogs, depending on the impl used.

        Also I don't see changes to CdcrTransactionLog like you mentioned - was that a false positive in your previous comment?

        Show
        Mike Drob added a comment - Not sure it makes sense to always make these cached boolean private static final . UpdateLog.preSoftCommit would refresh the values, I think HdfsUpdateLog now misses out on that, haven't confirmed though. Logging levels can change dynamically via the Solr UI or through file watchdogs, depending on the impl used. Also I don't see changes to CdcrTransactionLog like you mentioned - was that a false positive in your previous comment?
        Hide
        Jason Gerlowski added a comment - - edited

        Ah, I forgot to save in Eclipse before making the patch. Find the updated version attached.

        I see your point about the cached debug variables not getting refreshed ever. UpdateLog.preSoftCommit() will update the cached value in the abstract parent, but won't affect the values in HdfsUpdateLog etc.

        One way to fix this would be to implement a preSoftCommit() method in each of this children which resets the cached value in the child before calling super.preSoftCommit(). Not sure if there's a better solution though. It seems like there should be a better way to do this...

        Show
        Jason Gerlowski added a comment - - edited Ah, I forgot to save in Eclipse before making the patch. Find the updated version attached. I see your point about the cached debug variables not getting refreshed ever. UpdateLog.preSoftCommit() will update the cached value in the abstract parent, but won't affect the values in HdfsUpdateLog etc. One way to fix this would be to implement a preSoftCommit() method in each of this children which resets the cached value in the child before calling super.preSoftCommit() . Not sure if there's a better solution though. It seems like there should be a better way to do this...
        Hide
        Jason Gerlowski added a comment -

        I've attached a version of the patch (SOLR-8359-nonfinal-values.patch) which allows the cached debug value in HdfsUpdateLog to be updated on each call to preSoftCommit.

        Not sure that we want to take the approach I mentioned for this, but I couldn't think of any other way to do it, so I figured I'd upload it so people can at least offer feedback/suggest alternatives.

        Show
        Jason Gerlowski added a comment - I've attached a version of the patch ( SOLR-8359 -nonfinal-values.patch) which allows the cached debug value in HdfsUpdateLog to be updated on each call to preSoftCommit . Not sure that we want to take the approach I mentioned for this, but I couldn't think of any other way to do it, so I figured I'd upload it so people can at least offer feedback/suggest alternatives.
        Hide
        Mike Drob added a comment -

        +1 LGTM

        Show
        Mike Drob added a comment - +1 LGTM
        Hide
        Anshum Gupta added a comment -

        Thanks Jason and Mike. I'll take a look at this.

        Show
        Anshum Gupta added a comment - Thanks Jason and Mike. I'll take a look at this.
        Hide
        ASF subversion and git services added a comment -

        Commit 1718257 from Anshum Gupta in branch 'dev/trunk'
        [ https://svn.apache.org/r1718257 ]

        SOLR-8359: Restrict child classes from using parent logger's state

        Show
        ASF subversion and git services added a comment - Commit 1718257 from Anshum Gupta in branch 'dev/trunk' [ https://svn.apache.org/r1718257 ] SOLR-8359 : Restrict child classes from using parent logger's state
        Hide
        ASF subversion and git services added a comment -

        Commit 1718259 from Anshum Gupta in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1718259 ]

        SOLR-8359: Restrict child classes from using parent logger's state (merge from trunk)

        Show
        ASF subversion and git services added a comment - Commit 1718259 from Anshum Gupta in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1718259 ] SOLR-8359 : Restrict child classes from using parent logger's state (merge from trunk)
        Hide
        Yonik Seeley added a comment -

        Why were the non-statics changed to statics?
        I just ran across an issue w/ peersync where I couldn't change the debug level dynamically.

        Show
        Yonik Seeley added a comment - Why were the non-statics changed to statics? I just ran across an issue w/ peersync where I couldn't change the debug level dynamically.
        Hide
        Yonik Seeley added a comment -

        Here's a patch that removes the "static" from these variables.

        Show
        Yonik Seeley added a comment - Here's a patch that removes the "static" from these variables.
        Hide
        ASF subversion and git services added a comment -

        Commit 1726045 from Yonik Seeley in branch 'dev/trunk'
        [ https://svn.apache.org/r1726045 ]

        SOLR-8359: revert change of cached debug levels to statics

        Show
        ASF subversion and git services added a comment - Commit 1726045 from Yonik Seeley in branch 'dev/trunk' [ https://svn.apache.org/r1726045 ] SOLR-8359 : revert change of cached debug levels to statics

          People

          • Assignee:
            Unassigned
            Reporter:
            Mike Drob
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development