Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-6090

Remove unreachable printLayout usage in cloud test

    Details

    • Type: Task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.3, master (7.0)
    • Component/s: SolrCloud, Tests
    • Labels:
      None

      Description

      Many cloud tests have a DEBUG instance variable e.g.

      private static final boolean DEBUG = false;
      

      and

      if (DEBUG) {
            super.printLayout();
      }
      

      I cannot find where this variable is set to true so the printLayout is never actually printed.

      We need to review and fix all such tests:

      1. BasicDistributedZKTest.doTest
      2. CollectionsAPIAsyncDistributedZkTest.doTest
      3. CollectionsAPIDistributedZkTest
      4. CustomCollectionTest
      5. UnloadDistributedZkTest

      This is not an exhaustive list.

      1. SOLR-6090.patch
        13 kB
        Cao Manh Dat

        Activity

        Hide
        caomanhdat Cao Manh Dat added a comment -

        This patch delete some places that have above boilerplate code.

        Show
        caomanhdat Cao Manh Dat added a comment - This patch delete some places that have above boilerplate code.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 3eb0f7c79286e9ab4f0c24b7f2338e1a35c833c9 in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3eb0f7c ]

        SOLR-6090: Remove unreachable printLayout usage in cloud tests

        Show
        jira-bot ASF subversion and git services added a comment - Commit 3eb0f7c79286e9ab4f0c24b7f2338e1a35c833c9 in lucene-solr's branch refs/heads/master from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3eb0f7c ] SOLR-6090 : Remove unreachable printLayout usage in cloud tests
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 2124f1250cc3e9a2ef93d41c7f968864d6f794e7 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2124f12 ]

        SOLR-6090: Remove unreachable printLayout usage in cloud tests

        (cherry picked from commit 3eb0f7c)

        Show
        jira-bot ASF subversion and git services added a comment - Commit 2124f1250cc3e9a2ef93d41c7f968864d6f794e7 in lucene-solr's branch refs/heads/branch_6x from Shalin Shekhar Mangar [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2124f12 ] SOLR-6090 : Remove unreachable printLayout usage in cloud tests (cherry picked from commit 3eb0f7c)
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Thanks Dat!

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Thanks Dat!
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        I cannot find where this variable is set to true so the printLayout is never actually printed.

        Normally, stuff like this is set manually by someone trying to get more info when trying to debug a failing test.

        Here's another example: StoppableIndexingThread has a parameter called doDeletes... but every case in the code base passes "true". But that doesn't mean it's a useless parameter and should be removed. One of the first things I do when trying to debug a Chaos* fail is to set that parameter to false and see if the error happens with adds only. It simplifies debugging.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - I cannot find where this variable is set to true so the printLayout is never actually printed. Normally, stuff like this is set manually by someone trying to get more info when trying to debug a failing test. Here's another example: StoppableIndexingThread has a parameter called doDeletes... but every case in the code base passes "true". But that doesn't mean it's a useless parameter and should be removed. One of the first things I do when trying to debug a Chaos* fail is to set that parameter to false and see if the error happens with adds only. It simplifies debugging.
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        I can understand the point of doDeletes but this stuff is trivial. Log it in debug level or add it when necessary otherwise it is just code smell to me. Anyway, if you want to keep it, feel free to revert my commit.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - I can understand the point of doDeletes but this stuff is trivial. Log it in debug level or add it when necessary otherwise it is just code smell to me. Anyway, if you want to keep it, feel free to revert my commit.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        Log it in debug level or add it when necessary

        Perhaps better way to phase the summary in this issue would be "Should this be hooked into a debugging level, or is it no longer useful and can be removed?"

        Anyway, if you want to keep it, feel free to revert my commit.

        That debugging code wasn't mine and I've never used it.
        I was more responding to the original reasoning for removal (that it was dead code).
        I guess there's a lot of context and judgement to these things though... for example, is the person removing the code the person who wrote it? In that case, it does feel like a trivial cleanup (prob no one else ever used it) and wouldn't even warrant a JIRA IMO.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - Log it in debug level or add it when necessary Perhaps better way to phase the summary in this issue would be "Should this be hooked into a debugging level, or is it no longer useful and can be removed?" Anyway, if you want to keep it, feel free to revert my commit. That debugging code wasn't mine and I've never used it. I was more responding to the original reasoning for removal (that it was dead code). I guess there's a lot of context and judgement to these things though... for example, is the person removing the code the person who wrote it? In that case, it does feel like a trivial cleanup (prob no one else ever used it) and wouldn't even warrant a JIRA IMO.
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Closing after 6.3.0 release.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Closing after 6.3.0 release.

          People

          • Assignee:
            caomanhdat Cao Manh Dat
            Reporter:
            shalinmangar Shalin Shekhar Mangar
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development