[
https://issues.apache.org/jira/browse/SOLR-13544?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Gus Heck updated SOLR-13544:
----------------------------
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
{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?
was:
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?
> 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
> Priority: Trivial
>
> 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
> {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-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
