Solr
  1. Solr
  2. SOLR-8454

Improve logging by ZkStateReader and clean up dead code

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, 6.0
    • Component/s: SolrCloud
    • Labels:
      None
    • Flags:
      Patch

      Description

      Improve logging output by ZkStateReader, by adding the following:

      • Use LOG.foo() with parameters properly (i.e. not concatenating strings w/ +)
      • Surround parameters with [], to help readability, especially w/ empty values
      • Add missing string messages, where I felt a message will clarify
      • Convert some try-catch to a try-multicatch and improve output log message

      Also, clean up dead code.

      1. SOLR-8454.patch
        27 kB
        Anshum Gupta
      2. SOLR-8454.patch
        24 kB
        Shai Erera
      3. SOLR-8454.patch
        24 kB
        Shai Erera
      4. SOLR-8454.patch
        22 kB
        Shai Erera

        Activity

        Hide
        Anshum Gupta added a comment -

        Thanks for doing this. The usage of LOG vs log is kind of split in the code base and I don't have a strong opinion on that so that's ok.

        I see you've capitalized log messages i.e. upper case starting char, at all places but here

        LOG.debug("server older than client {}<{}", collection.getZNodeVersion(), version);
        

        The rest all looks good to me to commit.

        Show
        Anshum Gupta added a comment - Thanks for doing this. The usage of LOG vs log is kind of split in the code base and I don't have a strong opinion on that so that's ok. I see you've capitalized log messages i.e. upper case starting char, at all places but here LOG.debug( "server older than client {}<{}" , collection.getZNodeVersion(), version); The rest all looks good to me to commit.
        Hide
        Shai Erera added a comment -

        While I was at it, I removed some unused code, thrown exceptions etc. Those are minor additions over the previous patch.

        Show
        Shai Erera added a comment - While I was at it, I removed some unused code, thrown exceptions etc. Those are minor additions over the previous patch.
        Hide
        Shai Erera added a comment -

        Thanks Anshum, addressed your comment in this patch.

        Show
        Shai Erera added a comment - Thanks Anshum, addressed your comment in this patch.
        Hide
        Anshum Gupta added a comment -

        Updated patch.

        I've cleaned up more code in this patch while we are at it. We should however explicit mention that in the ticket summary that we are not only improving the logging here but also cleaning up code.

        Here are the things that I have changed:

        • Removed unused import for ThreadFactory
        • Removed the unwanted path param from addSecuritynodeWatcher() method. The path is always SOLR_SECURITY_CONF_PATH and so it makes sense to just directly use it.
        • In addSecuritynodeWatcher.process() removed the following code as those conditions are never true. The code block doesn't throw KeeperException or InterruptedException:
          if (e instanceof KeeperException) throw (KeeperException) e;
          if (e instanceof InterruptedException) throw (InterruptedException) e;
          
        • Capitalized the log line I mentioned in my last comment.
        Show
        Anshum Gupta added a comment - Updated patch. I've cleaned up more code in this patch while we are at it. We should however explicit mention that in the ticket summary that we are not only improving the logging here but also cleaning up code. Here are the things that I have changed: Removed unused import for ThreadFactory Removed the unwanted path param from addSecuritynodeWatcher() method. The path is always SOLR_SECURITY_CONF_PATH and so it makes sense to just directly use it. In addSecuritynodeWatcher.process() removed the following code as those conditions are never true. The code block doesn't throw KeeperException or InterruptedException: if (e instanceof KeeperException) throw (KeeperException) e; if (e instanceof InterruptedException) throw (InterruptedException) e; Capitalized the log line I mentioned in my last comment.
        Hide
        ASF subversion and git services added a comment -

        Commit 1721393 from Shai Erera in branch 'dev/trunk'
        [ https://svn.apache.org/r1721393 ]

        SOLR-8454: Improve logging by ZkStateReader and clean up dead code

        Show
        ASF subversion and git services added a comment - Commit 1721393 from Shai Erera in branch 'dev/trunk' [ https://svn.apache.org/r1721393 ] SOLR-8454 : Improve logging by ZkStateReader and clean up dead code
        Hide
        ASF subversion and git services added a comment -

        Commit 1721397 from Shai Erera in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1721397 ]

        SOLR-8454: Improve logging by ZkStateReader and clean up dead code

        Show
        ASF subversion and git services added a comment - Commit 1721397 from Shai Erera in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1721397 ] SOLR-8454 : Improve logging by ZkStateReader and clean up dead code
        Hide
        Shai Erera added a comment -

        Thanks Anshum Gupta. Committed to trunk and 5x.

        Show
        Shai Erera added a comment - Thanks Anshum Gupta . Committed to trunk and 5x.
        Hide
        Anshum Gupta added a comment -

        Thanks Shai.
        Seems like you missed the Change log entry though. I'll add that.

        Show
        Anshum Gupta added a comment - Thanks Shai. Seems like you missed the Change log entry though. I'll add that.
        Hide
        ASF subversion and git services added a comment -

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

        SOLR-8454: Adding change log entry

        Show
        ASF subversion and git services added a comment - Commit 1721492 from Anshum Gupta in branch 'dev/trunk' [ https://svn.apache.org/r1721492 ] SOLR-8454 : Adding change log entry
        Hide
        ASF subversion and git services added a comment -

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

        SOLR-8454: Adding change log entry (merge from trunk)

        Show
        ASF subversion and git services added a comment - Commit 1721495 from Anshum Gupta in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1721495 ] SOLR-8454 : Adding change log entry (merge from trunk)

          People

          • Assignee:
            Shai Erera
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development