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

Check for null references that may be returned due to concurrent changes or inconsistent cluster state #7181

Merged
merged 1 commit into from Aug 11, 2014

Conversation

Projects
None yet
3 participants
@martijnvg
Copy link
Member

martijnvg commented Aug 6, 2014

No description provided.

@martijnvg martijnvg added the review label Aug 7, 2014

@jpountz

View changes

src/main/java/org/elasticsearch/indices/IndicesService.java Outdated
*
* Even if the index name appreared in {@link #indices()} <code>null</code> can still be returned as an
* index maybe removed in the meantime.
*/

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 11, 2014

Contributor

Thanks for adding javadocs

}
});
routingTableDirty = false;
} catch (Exception e) {
logger.warn("Failed to reroute routing table", e);
ClusterState state = clusterService.state();
logger.warn("Failed to reroute routing table, current state:\n{}", e, state.prettyPrint());

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 11, 2014

Contributor

I imagine this can generate pretty large logs? Is it ok?

This comment has been minimized.

Copy link
@martijnvg

martijnvg Aug 11, 2014

Author Member

This is intended, actually failures should be printed in InternalClusterService line 485, but because we catch the exception here we never print it.

If something went wrong here it is good to know what the cluster state is, so we can easier debug. This shouldn't be printed too often, but happens in case of unexpected exeception (NPE etc).

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 11, 2014

Contributor

ok

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 11, 2014

Since in both cases that you fixed, the code iterates over indices() and then gets the index service, I'm wondering that it might be less error-prone to make IndicesService expose a Map<String, IndexService> (by copying the one it has internally) so that there is no need for those null checks?

@jpountz jpountz removed the review label Aug 11, 2014

@martijnvg

This comment has been minimized.

Copy link
Member Author

martijnvg commented Aug 11, 2014

@jpountz I like that approach, I think we should make a copy and return ImmutableMap as well?

@martijnvg

This comment has been minimized.

Copy link
Member Author

martijnvg commented Aug 11, 2014

@jpountz I updated the PR with your suggestion and add more null checks for fetching IndexService where that was missing.

@jpountz

View changes

src/main/java/org/elasticsearch/action/explain/TransportExplainAction.java Outdated
if (indexService == null) {
// Will be retried higher up the stack
throw new IndexMissingException(new Index(request.index()));
}

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 11, 2014

Contributor

If you call indexServiceSafe I don't think you would need the null check?

This comment has been minimized.

Copy link
@martijnvg

martijnvg Aug 11, 2014

Author Member

doh, of course, I'll change that.

@jpountz

View changes

src/main/java/org/elasticsearch/action/explain/TransportExplainAction.java Outdated
if (indexShard == null) {
// Will be retried higher up the stack
throw new IndexShardMissingException(new ShardId(request.index(), shardId));
}

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 11, 2014

Contributor

Should be unnecessary since shardSafe is called? (as opposed to just shard)

@jpountz

View changes

src/main/java/org/elasticsearch/indices/IndicesService.java Outdated
*
* Even if the index name appeared in {@link #indices()} <code>null</code> can still be returned as an
* index maybe removed in the meantime, so preferable use the associated {@link IndexService} in order to prevent NPE.
*/
IndexService indexService(String index);

IndexService indexServiceSafe(String index) throws IndexMissingException;

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 11, 2014

Contributor

Would be nice to have some documentation on this one as well :)

@jpountz

View changes

src/main/java/org/elasticsearch/indices/cluster/IndicesClusterStateService.java Outdated
for (Map.Entry<String, IndexService> entry : indicesService.indices().entrySet()) {
String index = entry.getKey();
IndexService indexService = entry.getValue();
if (indexService != null && indexService.shardIds().isEmpty()) {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 11, 2014

Contributor

The null check looks useless?

@jpountz

View changes

src/main/java/org/elasticsearch/indices/IndicesService.java Outdated
/**
* Returns the names of indices and associated {@link IndexService} instances that are started and active.
*/
ImmutableMap<String, IndexService> indices();

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 11, 2014

Contributor

Can you document that it is a snapshot (as opposed to a realtime view)?

@jpountz

View changes

src/main/java/org/elasticsearch/indices/InternalIndicesService.java Outdated
public Set<String> indices() {
return newHashSet(indices.keySet());
public ImmutableMap<String, IndexService> indices() {
return ImmutableMap.copyOf(indices);

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 11, 2014

Contributor

Is the copy necessary? (Since indices is already immutable)

This comment has been minimized.

Copy link
@martijnvg

martijnvg Aug 11, 2014

Author Member

good point, I'll change that.

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 11, 2014

This looks great, thanks @martijnvg

Core: Avoid null references that may be returned due to concurrent ch…
…anges or inconsistent cluster state

Closes #7181

martijnvg added a commit that referenced this pull request Aug 11, 2014

@martijnvg martijnvg merged commit 565dd90 into elastic:master Aug 11, 2014

martijnvg added a commit that referenced this pull request Aug 11, 2014

Core: Avoid null references that may be returned due to concurrent ch…
…anges or inconsistent cluster state

Closes #7181

martijnvg added a commit that referenced this pull request Sep 8, 2014

@clintongormley clintongormley changed the title Core: Verify for null references that may be returned due to concurrent changes or inconsistent cluster state Internal: Check for null references that may be returned due to concurrent changes or inconsistent cluster state Sep 8, 2014

@martijnvg martijnvg deleted the martijnvg:bugs/incosistent-state-checking branch May 18, 2015

@clintongormley clintongormley changed the title Internal: Check for null references that may be returned due to concurrent changes or inconsistent cluster state Check for null references that may be returned due to concurrent changes or inconsistent cluster state Jun 7, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.