Solr
  1. Solr
  2. SOLR-7147

Introduce new TrackingShardHandlerFactory for monitoring what requests are sent to shards during tests

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.10.4, 5.1, 6.0
    • Component/s: SolrCloud, Tests
    • Labels:
      None

      Description

      this is an idea shalin proposed as part of the testing for SOLR-7128...

      I created a TrackingShardHandlerFactory which can record shard requests sent from any node. There are a few helper methods to get requests by shard and by purpose.

      ...

      I will likely move the TrackingShardHandlerFactory into its own issue because it is helpful for other distributed tests as well. I also need to decouple it from the MiniSolrCloudCluster abstraction.

      1. SOLR-7147.patch
        28 kB
        Shalin Shekhar Mangar
      2. SOLR-7147.patch
        34 kB
        Shalin Shekhar Mangar
      3. SOLR-7147.patch
        28 kB
        Shalin Shekhar Mangar
      4. SOLR-7147.patch
        27 kB
        Shalin Shekhar Mangar
      5. SOLR-7147.patch
        8 kB
        Shalin Shekhar Mangar
      6. SOLR-7147.patch
        7 kB
        Shalin Shekhar Mangar

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          I created a TrackingShardHandlerFactory which can record shard requests sent from any node. There are a few helper methods to get requests by shard and by purpose.

          I like the idea ... hooking into the ShardHandlerFactory seems like a good solution to these types of problems...

          I will likely move the TrackingShardHandlerFactory into its own issue because it is helpful for other distributed tests as well.

          Yeah, a top level class in the test-framework would be good, but...

          I'm not sure if a Map<String, List<ShardRequestAndParams>> is really the best solution here?

          It's good enough for this small test case, but as a general solution ... consider Test Classes were folks want to setup their distrib cluster, do a handfull of specific queries to monitor what gets sent to each shard, and then in other test methods crank out thousands of distrib queries. the way this is setup, that's either going to eat up a ton of ram, or the test is going to eat up a ton of ram, or the test is going to need to be sprinkled with tons of calls to some clearOutShardHandlerTrackers() utility method

          perhaps instead, the TrackingShardHandlerFactory should have a setTrackingQueue/getTrackingQueue methods for a Queue<ShardRequestAndParams> (and ShardRequestAndParams should also have a String shard property - although isn't this already in the ShardRequest?).

          By default, the Queue is null and TrackingShardHandlerFactory doesn't track anything – but if it's non-null, then the ShardHandler calls add(new ShardRequestAndParams(...) just before passing executing it.

          this way, there is almost no overhead to using TrackingShardHandlerFactory unless/until a test explicitly specifies a Queue. Test classes, for the portions of the test when they want to monitor shard requests, could call setTrackingQueue on a custom subclass of AbstractQueue that directly asserts things like...

          if (req.sreq.purpose & PURPOSE_GET_TOP_IDS) {
            assertEquals(expected, req.sreq.getParams(FL));
          }
          

          .. alternatively, a LinkedBlockingQueue could be specified, and the test class could spin up an Executor that was would poll that Queue continuously inspecting them & recording info for processing/assert at the end of the test.

          at any point when the test case no longer cares about tracking, setTrackingQueue(null) again.

          The simplier use case of what you're doing here could still pretty easily be supported, just by providing a simple special case Queue class that has a Map<String, List<ShardRequestAndParams>> getAll() method on it, and tests could do something like...

            TrackAllQueue myQueue = TrackingShardHandlerFactory.setupTrackAllQueue(myMiniSolrCloudCluster, collectionName);
            // .. do test stuff ..
            TrackingShardHandlerFactory.setAllTrackingQueues(myMiniSolrCloudCluster, null)
            List<ShardRequestAndParams> list = myQueye.getAll().get(coreUrl);
            // ...
          

          I will likely move the TrackingShardHandlerFactory into its own issue because it is helpful for other distributed tests as well. I also need to decouple it from the MiniSolrCloudCluster abstraction.

          I think the main thing would be some static methods in TrackingShardHandlerFactory to get/set the TrackingQueue for all JettySolrRunners in a list, or on a MiniSolrCloudCluster (in which case it just pulls out the JettySolrRunners from there)

          what do you think?

          Show
          Hoss Man added a comment - I created a TrackingShardHandlerFactory which can record shard requests sent from any node. There are a few helper methods to get requests by shard and by purpose. I like the idea ... hooking into the ShardHandlerFactory seems like a good solution to these types of problems... I will likely move the TrackingShardHandlerFactory into its own issue because it is helpful for other distributed tests as well. Yeah, a top level class in the test-framework would be good, but... I'm not sure if a Map<String, List<ShardRequestAndParams>> is really the best solution here? It's good enough for this small test case, but as a general solution ... consider Test Classes were folks want to setup their distrib cluster, do a handfull of specific queries to monitor what gets sent to each shard, and then in other test methods crank out thousands of distrib queries. the way this is setup, that's either going to eat up a ton of ram, or the test is going to eat up a ton of ram, or the test is going to need to be sprinkled with tons of calls to some clearOutShardHandlerTrackers() utility method perhaps instead, the TrackingShardHandlerFactory should have a setTrackingQueue/getTrackingQueue methods for a Queue<ShardRequestAndParams> (and ShardRequestAndParams should also have a String shard property - although isn't this already in the ShardRequest?). By default, the Queue is null and TrackingShardHandlerFactory doesn't track anything – but if it's non-null, then the ShardHandler calls add(new ShardRequestAndParams(...) just before passing executing it. this way, there is almost no overhead to using TrackingShardHandlerFactory unless/until a test explicitly specifies a Queue. Test classes, for the portions of the test when they want to monitor shard requests, could call setTrackingQueue on a custom subclass of AbstractQueue that directly asserts things like... if (req.sreq.purpose & PURPOSE_GET_TOP_IDS) { assertEquals(expected, req.sreq.getParams(FL)); } .. alternatively, a LinkedBlockingQueue could be specified, and the test class could spin up an Executor that was would poll that Queue continuously inspecting them & recording info for processing/assert at the end of the test. at any point when the test case no longer cares about tracking, setTrackingQueue(null) again. The simplier use case of what you're doing here could still pretty easily be supported, just by providing a simple special case Queue class that has a Map<String, List<ShardRequestAndParams>> getAll() method on it, and tests could do something like... TrackAllQueue myQueue = TrackingShardHandlerFactory.setupTrackAllQueue(myMiniSolrCloudCluster, collectionName); // .. do test stuff .. TrackingShardHandlerFactory.setAllTrackingQueues(myMiniSolrCloudCluster, null ) List<ShardRequestAndParams> list = myQueye.getAll().get(coreUrl); // ... I will likely move the TrackingShardHandlerFactory into its own issue because it is helpful for other distributed tests as well. I also need to decouple it from the MiniSolrCloudCluster abstraction. I think the main thing would be some static methods in TrackingShardHandlerFactory to get/set the TrackingQueue for all JettySolrRunners in a list, or on a MiniSolrCloudCluster (in which case it just pulls out the JettySolrRunners from there) what do you think?
          Hide
          Shalin Shekhar Mangar added a comment -

          Here's a patch which implements the ideas that Hoss outlined:

          TrackingShardHandlerFactory has the following API:

          public synchronized void setTrackingQueue(Queue<ShardRequestAndParams> queue) {
              this.queue = queue;
            }
          
            public synchronized Queue<ShardRequestAndParams> getTrackingQueue() {
              return queue;
            }
          
            public synchronized boolean isTracking() {
              return queue != null;
            }
          

          Then there's a RequestTrackingQueue which has the following:

          public ShardRequestAndParams getShardRequestByPurpose(ZkStateReader zkStateReader, String collectionName, String shardId, int purpose);
          
          public List<ShardRequestAndParams> getShardRequests(ZkStateReader zkStateReader, String collectionName, String shardId);
          
          public Map<String, List<ShardRequestAndParams>> getAllRequests();
          
          public List<ShardRequestAndParams> getCollectionAdminRequests();
          
          public List<ShardRequestAndParams> getCoreAdminRequests();
          

          The getCollectionAdminRequests() and getCoreAdminRequests() methods are not implemented in this patch.

          Show
          Shalin Shekhar Mangar added a comment - Here's a patch which implements the ideas that Hoss outlined: TrackingShardHandlerFactory has the following API: public synchronized void setTrackingQueue(Queue<ShardRequestAndParams> queue) { this .queue = queue; } public synchronized Queue<ShardRequestAndParams> getTrackingQueue() { return queue; } public synchronized boolean isTracking() { return queue != null ; } Then there's a RequestTrackingQueue which has the following: public ShardRequestAndParams getShardRequestByPurpose(ZkStateReader zkStateReader, String collectionName, String shardId, int purpose); public List<ShardRequestAndParams> getShardRequests(ZkStateReader zkStateReader, String collectionName, String shardId); public Map< String , List<ShardRequestAndParams>> getAllRequests(); public List<ShardRequestAndParams> getCollectionAdminRequests(); public List<ShardRequestAndParams> getCoreAdminRequests(); The getCollectionAdminRequests() and getCoreAdminRequests() methods are not implemented in this patch.
          Hide
          Shalin Shekhar Mangar added a comment -

          Patch which adds following static helper methods in TrackingShardHandlerFactory to enable tracking on all nodes:

          public static void setTrackingQueue(MiniSolrCloudCluster cluster, Queue<ShardRequestAndParams> queue);
          
          public static void setTrackingQueue(List<JettySolrRunner> runners, Queue<ShardRequestAndParams> queue);
          

          I removed the proposed methods for tracking core admin and collection APIs. We can't track them with a shard handler factory and it is out of scope for this issue.

          I'm working on a test.

          Show
          Shalin Shekhar Mangar added a comment - Patch which adds following static helper methods in TrackingShardHandlerFactory to enable tracking on all nodes: public static void setTrackingQueue(MiniSolrCloudCluster cluster, Queue<ShardRequestAndParams> queue); public static void setTrackingQueue(List<JettySolrRunner> runners, Queue<ShardRequestAndParams> queue); I removed the proposed methods for tracking core admin and collection APIs. We can't track them with a shard handler factory and it is out of scope for this issue. I'm working on a test.
          Hide
          Shalin Shekhar Mangar added a comment -
          1. Added tests and javadocs
          2. Added a method to return all core admin requests invoked by the Collection API

          I think this is ready.

          Show
          Shalin Shekhar Mangar added a comment - Added tests and javadocs Added a method to return all core admin requests invoked by the Collection API I think this is ready.
          Hide
          Shalin Shekhar Mangar added a comment -

          I fixed some javadocs so that precommit could pass.

          Show
          Shalin Shekhar Mangar added a comment - I fixed some javadocs so that precommit could pass.
          Hide
          Shalin Shekhar Mangar added a comment -

          Added another test TestTrackingShardHandler2 which uses Solr's test harness instead of MiniSolrCloudCluster. The test is mostly a duplicate of TestTrackingShardHandler but I wanted to make sure it works with both MiniSolrCloudCluster and our test harness.

          Show
          Shalin Shekhar Mangar added a comment - Added another test TestTrackingShardHandler2 which uses Solr's test harness instead of MiniSolrCloudCluster. The test is mostly a duplicate of TestTrackingShardHandler but I wanted to make sure it works with both MiniSolrCloudCluster and our test harness.
          Hide
          Shalin Shekhar Mangar added a comment -

          I thought about the tests more and finally decided to remove the MiniSolrCloudCluster based test. We're not really doing anything more than testing the setTrackingQueue method that accepts MiniSolrCloudCluster which internally delegates to setTrackingQueue(List<JettySolrRunner>).

          This is ready. All tests pass. ant precommit passes. I'll commit this shortly.

          Show
          Shalin Shekhar Mangar added a comment - I thought about the tests more and finally decided to remove the MiniSolrCloudCluster based test. We're not really doing anything more than testing the setTrackingQueue method that accepts MiniSolrCloudCluster which internally delegates to setTrackingQueue(List<JettySolrRunner>). This is ready. All tests pass. ant precommit passes. I'll commit this shortly.
          Hide
          ASF subversion and git services added a comment -

          Commit 1662209 from shalin@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1662209 ]

          SOLR-7147: Introduce new TrackingShardHandlerFactory for monitoring what requests are sent to shards during tests

          Show
          ASF subversion and git services added a comment - Commit 1662209 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1662209 ] SOLR-7147 : Introduce new TrackingShardHandlerFactory for monitoring what requests are sent to shards during tests
          Hide
          ASF subversion and git services added a comment -

          Commit 1662210 from shalin@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1662210 ]

          SOLR-7147: Introduce new TrackingShardHandlerFactory for monitoring what requests are sent to shards during tests

          Show
          ASF subversion and git services added a comment - Commit 1662210 from shalin@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1662210 ] SOLR-7147 : Introduce new TrackingShardHandlerFactory for monitoring what requests are sent to shards during tests
          Hide
          ASF subversion and git services added a comment -

          Commit 1662300 from shalin@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1662300 ]

          SOLR-7147: Handle multiple replicas correctly in getShardRequests

          Show
          ASF subversion and git services added a comment - Commit 1662300 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1662300 ] SOLR-7147 : Handle multiple replicas correctly in getShardRequests
          Hide
          ASF subversion and git services added a comment -

          Commit 1662301 from shalin@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1662301 ]

          SOLR-7147: Handle multiple replicas correctly in getShardRequests

          Show
          ASF subversion and git services added a comment - Commit 1662301 from shalin@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1662301 ] SOLR-7147 : Handle multiple replicas correctly in getShardRequests
          Hide
          Otis Gospodnetic added a comment -

          Is this TrackingShardHandlerFactory really useful only for tests? Wouldn't this be a useful debugging tool in general?

          Show
          Otis Gospodnetic added a comment - Is this TrackingShardHandlerFactory really useful only for tests? Wouldn't this be a useful debugging tool in general?
          Hide
          Shalin Shekhar Mangar added a comment -

          Is this TrackingShardHandlerFactory really useful only for tests? Wouldn't this be a useful debugging tool in general?

          Yeah, it is horrible for any non-test usage. Once you set the tracking queue, it will hold all shard requests including their parameter and the responses in memory. So I don't see it being generally useful in it's current form.

          Show
          Shalin Shekhar Mangar added a comment - Is this TrackingShardHandlerFactory really useful only for tests? Wouldn't this be a useful debugging tool in general? Yeah, it is horrible for any non-test usage. Once you set the tracking queue, it will hold all shard requests including their parameter and the responses in memory. So I don't see it being generally useful in it's current form.
          Hide
          ASF subversion and git services added a comment -

          Commit 1662777 from shalin@apache.org in branch 'dev/branches/lucene_solr_4_10'
          [ https://svn.apache.org/r1662777 ]

          SOLR-7147: Introduce new TrackingShardHandlerFactory for monitoring what requests are sent to shards during tests

          Show
          ASF subversion and git services added a comment - Commit 1662777 from shalin@apache.org in branch 'dev/branches/lucene_solr_4_10' [ https://svn.apache.org/r1662777 ] SOLR-7147 : Introduce new TrackingShardHandlerFactory for monitoring what requests are sent to shards during tests
          Hide
          Michael McCandless added a comment -

          Bulk close for 4.10.4 release

          Show
          Michael McCandless added a comment - Bulk close for 4.10.4 release

            People

            • Assignee:
              Shalin Shekhar Mangar
              Reporter:
              Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development