Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-15963: Refactor code to minimize access to SolrCore #599

Merged
merged 2 commits into from
Feb 10, 2022

Conversation

noblepaul
Copy link
Contributor

No description provided.

@@ -225,7 +225,7 @@ public String computeInitialCollectionName() {
public CandidateCollection findCandidateGivenValue(AddUpdateCommand cmd) {
Object value = cmd.getSolrInputDocument().getFieldValue(getRouteField());
String targetColName = buildCollectionNameFromValue(String.valueOf(value));
ZkStateReader zkStateReader = cmd.getReq().getCore().getCoreContainer().getZkController().zkStateReader;
ZkStateReader zkStateReader = cmd.getReq().getCoreContainer().getZkController().zkStateReader;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by cmd.getReq().getCoreContainer() could be null and is dereferenced at line 228.
(at-me in a reply with help or ignore)

@@ -121,7 +121,7 @@ public RoutedAliasTypes getRoutedAliasType() {
@Override
public void validateRouteValue(AddUpdateCommand cmd) throws SolrException {
if (this.aliases == null) {
updateParsedCollectionAliases(cmd.getReq().getCore().getCoreContainer().getZkController().zkStateReader, false);
updateParsedCollectionAliases(cmd.getReq().getCoreContainer().getZkController().zkStateReader, false);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by cmd.getReq().getCoreContainer() could be null and is dereferenced at line 124.
(at-me in a reply with help or ignore)

@@ -449,7 +449,7 @@ private Instant parseRouteKey(Object routeKey) {
@Override
public CandidateCollection findCandidateGivenValue(AddUpdateCommand cmd) {
Object value = cmd.getSolrInputDocument().getFieldValue(getRouteField());
ZkStateReader zkStateReader = cmd.getReq().getCore().getCoreContainer().getZkController().zkStateReader;
ZkStateReader zkStateReader = cmd.getReq().getCoreContainer().getZkController().zkStateReader;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by cmd.getReq().getCoreContainer() could be null and is dereferenced at line 452.
(at-me in a reply with help or ignore)

ConfigOverlay.NAME,
latestVersion, 30);
} else {
SolrResourceLoader.persistConfLocally(loader, ConfigOverlay.RESOURCE_NAME, overlay.toByteArray());
req.getCore().getCoreContainer().reload(req.getCore().getName(), req.getCore().uniqueId);
req.getCoreContainer().reload(req.getCore().getName(), req.getCore().uniqueId);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by SolrConfigHandler$Command.req.getCoreContainer() could be null and is dereferenced at line 540.
(at-me in a reply with help or ignore)

@@ -472,7 +472,7 @@ private void handleParams(ArrayList<CommandOperation> ops, RequestParams params)

log.debug("persisted to version : {} ", latestVersion);
waitForAllReplicasState(req.getCore().getCoreDescriptor().getCloudDescriptor().getCollectionName(),
req.getCore().getCoreContainer().getZkController(), RequestParams.NAME, latestVersion, 30);
req.getCoreContainer().getZkController(), RequestParams.NAME, latestVersion, 30);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by SolrConfigHandler$Command.req.getCoreContainer() could be null and is dereferenced at line 475.
(at-me in a reply with help or ignore)

@@ -139,7 +139,7 @@ public void init(NamedList<?> args) {
public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp)
throws InterruptedException, KeeperException, IOException {

CoreContainer coreContainer = req.getCore().getCoreContainer();
CoreContainer coreContainer = req.getCoreContainer();
if (coreContainer.isZooKeeperAware()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object coreContainer last assigned on line 142 could be null and is dereferenced at line 143.
(at-me in a reply with help or ignore)

@@ -132,7 +132,7 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
addMBean(req, cats, requestedKeys, entry.getKey(),entry.getValue());
}

for (SolrInfoBean infoMBean : req.getCore().getCoreContainer().getResourceLoader().getInfoMBeans()) {
for (SolrInfoBean infoMBean : req.getCoreContainer().getResourceLoader().getInfoMBeans()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by req.getCoreContainer() could be null and is dereferenced at line 135.
(at-me in a reply with help or ignore)


final ReplicaListTransformer replicaListTransformer = httpShardHandlerFactory.getReplicaListTransformer(req);

AllowListUrlChecker urlChecker = req.getCore().getCoreContainer().getAllowListUrlChecker();
AllowListUrlChecker urlChecker = req.getCoreContainer().getAllowListUrlChecker();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by req.getCoreContainer() could be null and is dereferenced at line 270.
(at-me in a reply with help or ignore)

@@ -342,7 +342,7 @@ protected ReplicaListTransformer getReplicaListTransformer(final SolrQueryReques
final SolrParams params = req.getParams();
final SolrCore core = req.getCore(); // explicit check for null core (temporary?, for tests)
@SuppressWarnings("resource")
ZkController zkController = core == null ? null : core.getCoreContainer().getZkController();
ZkController zkController = req.getCoreContainer().getZkController();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by req.getCoreContainer() could be null and is dereferenced at line 345.
(at-me in a reply with help or ignore)

@@ -1569,8 +1569,8 @@ private void doProcessUngroupedSearch(ResponseBuilder rb, QueryCommand cmd, Quer
}

private static String generateQueryID(SolrQueryRequest req) {
ZkController zkController = req.getCore().getCoreContainer().getZkController();
String nodeName = req.getCore().getCoreContainer().getHostName();
ZkController zkController = req.getCoreContainer().getZkController();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by req.getCoreContainer() could be null and is dereferenced at line 1572.
(at-me in a reply with help or ignore)

@@ -288,7 +288,7 @@ protected void prepareGrouping(ResponseBuilder rb) throws IOException {
groupingSpec.setResponseFormat(responseFormat);

// See SOLR-12249. Disallow grouping on text fields that are not SortableText in cloud mode
if (req.getCore().getCoreContainer().isZooKeeperAware()) {
if (req.getCoreContainer().isZooKeeperAware()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by req.getCoreContainer() could be null and is dereferenced at line 291.
(at-me in a reply with help or ignore)

@@ -585,7 +585,7 @@ public static String getOrGenerateRequestId(SolrQueryRequest req) {
}

private static String generateRid(SolrQueryRequest req) {
String hostName = req.getCore().getCoreContainer().getHostName();
String hostName = req.getCoreContainer().getHostName();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by req.getCoreContainer() could be null and is dereferenced at line 588.
(at-me in a reply with help or ignore)

@@ -236,7 +236,7 @@ private void initComponents() {
public ShardHandler getAndPrepShardHandler(SolrQueryRequest req, ResponseBuilder rb) {
ShardHandler shardHandler = null;

CoreContainer cc = req.getCore().getCoreContainer();
CoreContainer cc = req.getCoreContainer();
boolean isZkAware = cc.isZooKeeperAware();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object cc last assigned on line 239 could be null and is dereferenced at line 240.
(at-me in a reply with help or ignore)

@@ -132,7 +132,7 @@ protected void processRequest(SolrQueryRequest req, ResponseBuilder rb, Map<Stri

public static ResponseBuilder buildResponseBuilder(SolrQueryRequest req, SolrQueryResponse rsp,
List<SearchComponent> components) {
CoreContainer cc = req.getCore().getCoreContainer();
CoreContainer cc = req.getCoreContainer();
boolean isZkAware = cc.isZooKeeperAware();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object cc last assigned on line 135 could be null and is dereferenced at line 136.
(at-me in a reply with help or ignore)

@@ -167,7 +167,7 @@ public SimpleFacets(SolrQueryRequest req,
this.docsOrig = docs;
this.global = params;
this.rb = rb;
this.facetExecutor = req.getCore().getCoreContainer().getUpdateShardHandler().getUpdateExecutor();
this.facetExecutor = req.getCoreContainer().getUpdateShardHandler().getUpdateExecutor();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by req.getCoreContainer() could be null and is dereferenced at line 170.
(at-me in a reply with help or ignore)

@@ -137,7 +137,7 @@ JoinParams parseJoin(QParser qparser) throws SyntaxError {
long fromCoreOpenTime = 0;

if (fromIndex != null && !fromIndex.equals(qparser.req.getCore().getCoreDescriptor().getName()) ) {
CoreContainer container = qparser.req.getCore().getCoreContainer();
CoreContainer container = qparser.req.getCoreContainer();

// if in SolrCloud mode, fromIndex should be the name of a single-sharded collection
coreName = ScoreJoinQParserPlugin.getCoreName(fromIndex, container);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object container last assigned on line 140 could be null and is dereferenced by call to getCoreName(...) at line 143.
(at-me in a reply with help or ignore)

@@ -228,7 +228,7 @@ private Query createQuery(final String fromField, final String fromQueryStr,
final String myCore = req.getCore().getCoreDescriptor().getName();

if (fromIndex != null && (!fromIndex.equals(myCore) || byPassShortCircutCheck)) {
CoreContainer container = req.getCore().getCoreContainer();
CoreContainer container = req.getCoreContainer();

final String coreName = getCoreName(fromIndex, container);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object container last assigned on line 231 could be null and is dereferenced by call to getCoreName(...) at line 233.
(at-me in a reply with help or ignore)

@@ -90,7 +90,7 @@ public OtherCoreJoinQuery(Query fromQuery, String fromField,
public Weight createWeight(IndexSearcher searcher, org.apache.lucene.search.ScoreMode scoreMode, float boost) throws IOException {
SolrRequestInfo info = SolrRequestInfo.getRequestInfo();

CoreContainer container = info.getReq().getCore().getCoreContainer();
CoreContainer container = info.getReq().getCoreContainer();

final SolrCore fromCore = container.getCore(fromIndex);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by getRequestInfo().getReq().getCoreContainer() could be null and is dereferenced at line 95.
(at-me in a reply with help or ignore)

@@ -1840,7 +1840,7 @@ public void doReplay(TransactionLog translog) {

UpdateRequestProcessorChain processorChain = req.getCore().getUpdateProcessingChain(null);
UpdateRequestProcessor proc = processorChain.createProcessor(req, rsp);
OrderedExecutor executor = inSortedOrder ? null : req.getCore().getCoreContainer().getReplayUpdatesExecutor();
OrderedExecutor executor = inSortedOrder ? null : req.getCoreContainer().getReplayUpdatesExecutor();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by UpdateLog$LogReplayer.req.getCoreContainer() could be null and is dereferenced at line 1843.
(at-me in a reply with help or ignore)

@@ -631,7 +631,7 @@ private long doWaitForDependentUpdates(AddUpdateCommand cmd, long versionOnUpdat
*/
private UpdateCommand fetchFullUpdateFromLeader(AddUpdateCommand inplaceAdd, long versionOnUpdate) throws IOException {
String id = inplaceAdd.getIndexedIdStr();
UpdateShardHandler updateShardHandler = inplaceAdd.getReq().getCore().getCoreContainer().getUpdateShardHandler();
UpdateShardHandler updateShardHandler = inplaceAdd.getReq().getCoreContainer().getUpdateShardHandler();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by inplaceAdd.getReq().getCoreContainer() could be null and is dereferenced at line 634.
(at-me in a reply with help or ignore)

@@ -50,7 +50,7 @@ public static void addParamToDistributedRequestWhitelist(final SolrQueryRequest
public UpdateRequestProcessor getInstance(SolrQueryRequest req,
SolrQueryResponse rsp, UpdateRequestProcessor next) {

final boolean isZkAware = req.getCore().getCoreContainer().isZooKeeperAware();
final boolean isZkAware = req.getCoreContainer().isZooKeeperAware();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by req.getCoreContainer() could be null and is dereferenced at line 53.
(at-me in a reply with help or ignore)

@@ -111,7 +111,7 @@
public DistributedZkUpdateProcessor(SolrQueryRequest req,
SolrQueryResponse rsp, UpdateRequestProcessor next) {
super(req, rsp, next);
CoreContainer cc = req.getCore().getCoreContainer();
CoreContainer cc = req.getCoreContainer();
cloudDesc = req.getCore().getCoreDescriptor().getCloudDescriptor();
zkController = cc.getZkController();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object cc last assigned on line 114 could be null and is dereferenced at line 116.
(at-me in a reply with help or ignore)

@@ -1260,7 +1260,7 @@ private void zkCheck() {
// Streaming updates can delay shutdown and cause big update reorderings (new streams can't be
// initiated, but existing streams carry on). This is why we check if the CC is shutdown.
// See SOLR-8203 and loop HdfsChaosMonkeyNothingIsSafeTest (and check for inconsistent shards) to test.
if (req.getCore().getCoreContainer().isShutDown()) {
if (req.getCoreContainer().isShutDown()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by DistributedUpdateProcessor.req.getCoreContainer() could be null and is dereferenced at line 1263.
(at-me in a reply with help or ignore)

@@ -127,7 +127,7 @@ public static UpdateRequestProcessor wrap(SolrQueryRequest req, UpdateRequestPro
}

private static Map<String, String> getAliasProps(SolrQueryRequest req, String aliasName) {
ZkController zkController = req.getCore().getCoreContainer().getZkController();
ZkController zkController = req.getCoreContainer().getZkController();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by req.getCoreContainer() could be null and is dereferenced at line 130.
(at-me in a reply with help or ignore)

@@ -134,7 +134,7 @@ public TolerantUpdateProcessor(SolrQueryRequest req, SolrQueryResponse rsp, Upda
this.distribPhase = distribPhase;
assert ! DistribPhase.FROMLEADER.equals(distribPhase);

this.zkController = this.req.getCore().getCoreContainer().getZkController();
this.zkController = this.req.getCoreContainer().getZkController();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by TolerantUpdateProcessor.req.getCoreContainer() could be null and is dereferenced at line 137.
(at-me in a reply with help or ignore)

@@ -109,7 +109,7 @@ public int distributedProcess(ResponseBuilder rb) throws IOException {

// Send out a request to each shard and merge the responses into our AnalyticsRequestManager
reqManager.shardStream.sendRequests(rb.req.getCore().getCoreDescriptor().getCollectionName(),
rb.req.getCore().getCoreContainer().getZkController().getZkServerAddress());
rb.req.getCoreContainer().getZkController().getZkServerAddress());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by rb.req.getCoreContainer() could be null and is dereferenced at line 112.
(at-me in a reply with help or ignore)

@@ -216,7 +216,7 @@ public void setContext(ResultContext context) {
}
leafContexts = searcher.getTopReaderContext().leaves();
if (threadManager != null) {
threadManager.setExecutor(context.getRequest().getCore().getCoreContainer().getUpdateShardHandler().getUpdateExecutor());
threadManager.setExecutor(context.getRequest().getCoreContainer().getUpdateShardHandler().getUpdateExecutor());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_DEREFERENCE: object returned by context.getRequest().getCoreContainer() could be null and is dereferenced at line 219.
(at-me in a reply with help or ignore)

Copy link
Contributor

@madrob madrob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include the JIRA number in the issue description.

The JIRA states "This also should help us provide alternate impls which do not require SolrCore access" - can this PR be expanded to include such an alternate implementation?

There are lots of places where we are not null safe now (we might not have been null safe before either). If we're cleaning this code up, we can consider returning an Optional? Not the only idea, there's probably other ways to make this code better for nulls.

@noblepaul noblepaul changed the title Refactor code to minimize access to SolrCore SOLR-15963: Refactor code to minimize access to SolrCore Feb 9, 2022
@noblepaul
Copy link
Contributor Author

can this PR be expanded to include such an alternate implementation?

@madrob , it's coming up in the parent ticket SOLR-15963

@dsmiley
Copy link
Contributor

dsmiley commented Feb 9, 2022

I'm fine with this without a JIRA; it's a pure refactoring -- very simple without any need for an over-arching purpose. I don't agree with use of Optional for request.getCoreContainer, which is all that this PR seeks to address. I like that this PR is laser focused and easy to review; thanks for doing this Noble!

@dsmiley
Copy link
Contributor

dsmiley commented Feb 9, 2022

+1 to merge, BTW. no CHANGES.txt needed either (in my mind, that's closer to when a JIRA is needed)

@noblepaul
Copy link
Contributor Author

1 to merge, BTW. no CHANGES.txt

Thanks @dsmiley

@noblepaul noblepaul merged commit 0cdb23b into main Feb 10, 2022
@cpoerschke cpoerschke deleted the jira/jira/solr15963_1 branch February 14, 2022 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants