lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gus Heck (JIRA)" <j...@apache.org>
Subject [jira] [Created] (SOLR-13544) Consider adding isCloud() method as shortcut for cloudDescriptor == null checks
Date Thu, 13 Jun 2019 06:18:00 GMT
Gus Heck created SOLR-13544:
-------------------------------

             Summary: Consider adding isCloud() method as shortcut for cloudDescriptor ==
null checks
                 Key: SOLR-13544
                 URL: https://issues.apache.org/jira/browse/SOLR-13544
             Project: Solr
          Issue Type: Improvement
      Security Level: Public (Default Security Level. Issues are Public)
          Components: SolrCloud
            Reporter: Gus Heck


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 MDCLoggingContext.java I see:
{code:java}
    // If this is a replica of type PULL, don't create the update log
    boolean skipUpdateLog = core.getCoreDescriptor().getCloudDescriptor() != null &&
!core.getCoreDescriptor().getCloudDescriptor().requiresTransactionLog();
{code}
This would be more concise and readable as:
{code:java}
    boolean skipUpdateLog = core.isCloud() && !core.getCoreDescriptor().getCloudDescriptor().requiresTransactionLog();
{code}
Or in the routed alias code:
{code:java}
    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 ...{code}
vs this which seems to read much smoother...
{code:java}
    SolrCore core = req.getCore();
    if (core.isCloud()) {
      CoreDescriptor coreDescriptor = core.getCoreDescriptor();
      CloudDescriptor cloudDescriptor = coreDescriptor.getCloudDescriptor();
      String collectionName = cloudDescriptor.getCollectionName();
... etc ...
{code}
The idea would be to add to SolrCore this method
{code:java}
  public boolean isCloud() {
    return getCoreDescriptor().getCloudDescriptor() != null;
  } {code}
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?



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message