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

Expose ShardId via LeafReader rather than Directory API #8812

Merged
merged 2 commits into from Dec 8, 2014

Conversation

Projects
None yet
4 participants
@s1monw
Contributor

s1monw commented Dec 8, 2014

Today we try to fetch a shard Id for a given IndexReader / LeafReader
by walking it's tree until the lucene internal SegmentReader and then
casting the directory into a StoreDirecotory. This class is fully internal
to Elasticsearch and should not be exposed outside of the Store.

This commit makes StoreDirectory a private inner class and adds dedicated
ElasticsearchDirectoryReader / ElasticserachLeafReader exposing a ShardId
getter to obtain information about the shard the index / segment belogs to.

These classes can be used to expose other segment specific information in
the future more easily.

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Dec 8, 2014

Contributor

@mikemccand can you take another look?

Contributor

s1monw commented Dec 8, 2014

@mikemccand can you take another look?

@mikemccand

This comment has been minimized.

Show comment
Hide comment
@mikemccand

mikemccand Dec 8, 2014

Contributor

One small typo still, else LGTM. Thanks for simplifying the deletes logging!

Contributor

mikemccand commented Dec 8, 2014

One small typo still, else LGTM. Thanks for simplifying the deletes logging!

[STORE] Expose ShardId via LeafReader rather than Direcotry API
Today we try to fetch a shard Id for a given IndexReader / LeafReader
by walking it's tree until the lucene internal SegmentReader and then
casting the directory into a StoreDirecotory. This class is fully internal
to Elasticsearch and should not be exposed outside of the Store.

This commit makes StoreDirectory a private inner class and adds dedicated
ElasticsearchDirectoryReader / ElasticserachLeafReader exposing a ShardId
getter to obtain information about the shard the index / segment belogs to.

These classes can be used to expose other segment specific information in
the future more easily.

@s1monw s1monw merged commit 8d7ce3c into elastic:master Dec 8, 2014

* A {@link org.apache.lucene.index.FilterLeafReader} that exposes
* Elasticsearch internal per shard / index information like the shard ID.
*/
public final class ElasticsearchLeafReader extends FilterLeafReader {

This comment has been minimized.

@rmuir

rmuir Dec 8, 2014

Contributor

I think this needs to override getCoreCacheKey() & co, or things are not really working per-segment :)

From javadocs:
NOTE: If this FilterLeafReader does not change the content the contained reader, you could consider overriding
getCoreCacheKey() so that CachingWrapperFilter shares the same entries for this atomic reader
and the wrapped one. getCombinedCoreAndDeletesKey() could be overridden as well if the live docs are not changed either.

@rmuir

rmuir Dec 8, 2014

Contributor

I think this needs to override getCoreCacheKey() & co, or things are not really working per-segment :)

From javadocs:
NOTE: If this FilterLeafReader does not change the content the contained reader, you could consider overriding
getCoreCacheKey() so that CachingWrapperFilter shares the same entries for this atomic reader
and the wrapped one. getCombinedCoreAndDeletesKey() could be overridden as well if the live docs are not changed either.

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Dec 8, 2014

Contributor

Added comment: we should fix the method overrides here.

Contributor

rmuir commented Dec 8, 2014

Added comment: we should fix the method overrides here.

@mikemccand

This comment has been minimized.

Show comment
Hide comment
@mikemccand

mikemccand Dec 8, 2014

Contributor

we should fix the method overrides here.

+1

How come no tests caught this?

Contributor

mikemccand commented Dec 8, 2014

we should fix the method overrides here.

+1

How come no tests caught this?

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Dec 8, 2014

Contributor

I committed a fix. I suspect nothing tests these cache key methods. It may be that no tests failed... because they are all integration tests and only test core-listener methods, which I think is the mechanism that happens to be used today (which could easily change, and core cache key might be used elsewhere, etc)

Contributor

rmuir commented Dec 8, 2014

I committed a fix. I suspect nothing tests these cache key methods. It may be that no tests failed... because they are all integration tests and only test core-listener methods, which I think is the mechanism that happens to be used today (which could easily change, and core cache key might be used elsewhere, etc)

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Dec 8, 2014

Contributor

i reviewed more, getCoreCacheKey() is used e.g. as part of every filter key, so NRT was really broken with filter caching for example. I still dont think integration tests for that are the right way: better would be to have 'nrt is working' tests at a lower level, turning off merging and calling reopen and asserting the core cache key did not change for this filterreader, and then moving our way up...

Contributor

rmuir commented Dec 8, 2014

i reviewed more, getCoreCacheKey() is used e.g. as part of every filter key, so NRT was really broken with filter caching for example. I still dont think integration tests for that are the right way: better would be to have 'nrt is working' tests at a lower level, turning off merging and calling reopen and asserting the core cache key did not change for this filterreader, and then moving our way up...

@clintongormley clintongormley removed the review label Mar 19, 2015

@clintongormley clintongormley changed the title from [STORE] Expose ShardId via LeafReader rather than Direcotry API to Expose ShardId via LeafReader rather than Directory API Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment