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

Consolidate shard level abstractions #11847

Merged
merged 1 commit into from Jun 24, 2015

Conversation

@s1monw
Copy link
Contributor

commented Jun 24, 2015

This commit consolidates several abstractions on the shard level in
ordinary classes not managed by the shard level guice injector.

Several classes have been collapsed into IndexShard and IndexShardGatewayService
was cleaned up to be more lightweight and self-contained. It has also been moved into
the index.shard package and it's operation is renamed from recovery from "gateway" to recovery
from "store" or "shard_store".

@jpountz
jpountz reviewed Jun 24, 2015
View changes
core/src/main/java/org/elasticsearch/index/shard/IndexShard.java Outdated
public FilterCacheStats filterCacheStats() {
return shardFilterCache.stats();
}
public FilterCacheStats filterCacheStats() { return indicesFilterCache.getStats(shardId); }

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 24, 2015

Contributor

I personally don't mind having single-line bodies on the same line as the method definition but I know some other people would, so let's just indent it as usual?

@jpountz
jpountz reviewed Jun 24, 2015
View changes
core/src/main/java/org/elasticsearch/index/shard/IndexShard.java Outdated
public PercolatorQueriesRegistry percolateRegistry() {
return percolatorQueriesRegistry;
}
public PercolatorQueriesRegistry percolateRegistry() { return percolatorQueriesRegistry; }

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 24, 2015

Contributor

idem: put the return statement on a new line?

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2015

You need to update docs/reference/indices/recovery.asciidoc as well which contains a reference to GATEWAY instead of STORE. Otherwise LGTM, it's exciting to get this stuff out of the hands of Guice.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2015

pushed some changed - thanks @jpountz

@bleskes
bleskes reviewed Jun 24, 2015
View changes
core/src/main/java/org/elasticsearch/index/indexing/IndexSlowLog.java Outdated
public ShardSlowLogIndexingService(ShardId shardId, @IndexSettings Settings indexSettings, IndexSettingsService indexSettingsService) {
super(shardId, indexSettings);

IndexSlowLog(ESLogger logger, Settings indexSettings) {

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 24, 2015

Member

I think this changes the logger name we have. Before it was index.indexing.slowlog.index and now we will have index.indexing.index we should adapt the getLogger calls below.

@bleskes
bleskes reviewed Jun 24, 2015
View changes
core/src/main/java/org/elasticsearch/index/indexing/IndexSlowLog.java Outdated
@@ -38,7 +34,7 @@

/**
*/
public class ShardSlowLogIndexingService extends AbstractIndexShardComponent {
public class IndexSlowLog implements IndexSettingsService.Listener {

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 24, 2015

Member

This is still a shard level component. Can we call it ShardSlowLog?

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 24, 2015

Member

I see what now where this come from - we have IndexSlowLog and SearchSlowLog. To avoid this confusion in the future, can we call this one IndexingSlowLog?

@bleskes
bleskes reviewed Jun 24, 2015
View changes
core/src/main/java/org/elasticsearch/index/search/stats/SearchSlowLog.java Outdated
@Inject
public ShardSlowLogSearchService(ShardId shardId, @IndexSettings Settings indexSettings, IndexSettingsService indexSettingsService) {
super(shardId, indexSettings);
SearchSlowLog(ESLogger logger, Settings indexSettings) {

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 24, 2015

Member

same issue caused by the package change. I think we should keep the log name as index.search.slowlog . To do so it will require to move the SearchSlowLog to the index.search package, together with ShardSearchService . I think that's OK?

this.indicesFilterCache = indicesFilterCache;
this.shardQueryCache = new ShardQueryCache(shardId, indexSettings);
this.shardFieldData = new ShardFieldData();
this.percolatorQueriesRegistry = new PercolatorQueriesRegistry(shardId, indexSettings, queryParserService, indexingService, indicesLifecycle, mapperService, indexFieldDataService, shardPercolateService);

This comment has been minimized.

Copy link
@bleskes
bind(IndexShardGateway.class).asEagerSingleton();
bind(IndexShardGatewayService.class).asEagerSingleton();
bind(PercolatorQueriesRegistry.class).asEagerSingleton();
bind(StoreRecoveryService.class).asEagerSingleton();

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 24, 2015

Member

nice..

@bleskes

This comment has been minimized.

Copy link
Member

commented Jun 24, 2015

I did as sweep as well. Left minor comments.

@s1monw s1monw force-pushed the s1monw:simplify_shard_module branch Jun 24, 2015
@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2015

pushed a new commit @bleskes

@bleskes

This comment has been minimized.

Copy link
Member

commented Jun 24, 2015

awesome. LGTM

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2015

LGTM too

This commit consolidates several abstractions on the shard level in
ordinary classes not managed by the shard level guice injector.

Several classes have been collapsed into IndexShard and IndexShardGatewayService
was cleaned up to be more lightweight and self-contained. It has also been moved into
the index.shard package and it's operation is renamed from recovery from "gateway" to recovery
from "store" or "shard_store".

Closes #11847
@s1monw s1monw force-pushed the s1monw:simplify_shard_module branch to fcdcce3 Jun 24, 2015
@s1monw s1monw merged commit fcdcce3 into elastic:master Jun 24, 2015
1 check passed
1 check passed
CLA Commit author is a member of Elasticsearch
Details
@s1monw s1monw removed the review label Jun 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.