[SOLR-13544] Consider adding isCloud() method as shortcut for cloudDescriptor == null checks

• Type: Improvement
• Status: Open
• Priority: Trivial
• Resolution: Unresolved
• Component/s: SolrCloud
• Assignee: Unassigned
• Reporter: Gus Heck
• Created: 13/Jun/19 06:17
• Updated: 26/Jul/19 09:01

Description

In writing SOLR-13420 I was not supper happy with the verbosity of some of my changes, and I think a convenience method could improve code legibility.

After doing some searching I found that in the first 50ish places where getCloudDescriptor() is called. 8 of them are followed by a null check, which is generally necessitated in cases where the code might run in non-cloud environments. There are a great many places where null is not checked and cloud is assumed, presumably for reasons related to the nature of the code (i.e. cdcr, code in zkcontroller), but some code does check, and the ethos generally seems to be "do x if we're in cloud". 

For example in UpdateHandler I see

    // If this is a replica of type PULL, don't create the update log
    boolean skipUpdateLog = core.getCoreDescriptor().getCloudDescriptor() != null && !core.getCoreDescriptor().getCloudDescriptor().requiresTransactionLog();

This would be more concise and readable as:

    boolean skipUpdateLog = core.isCloud() && !core.getCoreDescriptor().getCloudDescriptor().requiresTransactionLog();

Or in the routed alias code:

    SolrCore core = req.getCore();
    CoreDescriptor coreDescriptor = core.getCoreDescriptor();
    CloudDescriptor cloudDescriptor = coreDescriptor.getCloudDescriptor();
    if (cloudDescriptor != null) {
      String collectionName = cloudDescriptor.getCollectionName();
      CoreContainer coreContainer = core.getCoreContainer();
      ZkController zkController = coreContainer.getZkController(); 
... etc ...

vs this which seems to read much smoother...

    SolrCore core = req.getCore();
    if (core.isCloud()) {
      CoreDescriptor coreDescriptor = core.getCoreDescriptor();
      CloudDescriptor cloudDescriptor = coreDescriptor.getCloudDescriptor();
      String collectionName = cloudDescriptor.getCollectionName();
... etc ...

The idea would be to add to SolrCore this method

  public boolean isCloud() {
    return getCoreDescriptor().getCloudDescriptor() != null;
  } 

We also might add some asserts in there for other things folks might want to assume to be true in the cloud case and javadoc what's reliable when this returns true, but keep it skinny in production. The key question is are there any cases where we're in solr cloud but getCloudDescriptor() is null? Seems hard to imagine, but what do others think?