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

Remove some Internal* abstractions #8904

Closed
wants to merge 1 commit into from

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Dec 11, 2014

We have lots of boilerplate code that is unnecessarily abstracting
services ie InternalIndexShard and IndexShard or InternalIndexService and
IndexService. It's enough to have concrete classes for these core classes.

@s1monw s1monw added v2.0.0-beta1 v1.5.0 >docs General docs changes >enhancement review and removed >docs General docs changes labels Dec 11, 2014
@nik9000
Copy link
Member

nik9000 commented Dec 11, 2014

Improves readability. +1

@@ -552,7 +549,7 @@ private void applyNewOrUpdatedShards(final ClusterChangedEvent event) throws Ela
continue;
}

InternalIndexShard indexShard = (InternalIndexShard) indexService.shard(shardId);
IndexShard indexShard = (IndexShard) indexService.shard(shardId);
Copy link
Contributor

Choose a reason for hiding this comment

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

The cast is useless now? (There are a couple of other places that have this unnecessary cast)

@rmuir
Copy link
Contributor

rmuir commented Dec 11, 2014

+1, thanks for doing this.

@jpountz
Copy link
Contributor

jpountz commented Dec 11, 2014

Same here, +1! The change looks good to me, I was looking for some usages of componentSettings that would break due to package name changes, but I could not find any. I think there are just a couple of unnecessary casts that can be removed now that there is a single impl.

@mikemccand
Copy link
Contributor

+1, wonderful!

@s1monw
Copy link
Contributor Author

s1monw commented Dec 11, 2014

removed unnecessary casts - I think it's ready - I wanna remove this soon since it's a large change

We have lots of boilerplate code that is unnecessarily abstracting
services ie InternalIndexShard and IndexShard or InternalIndexService and
IndexService. It's enough to have concrete classes for these core classes.
@s1monw s1monw closed this in 3877dc6 Dec 11, 2014
@s1monw s1monw deleted the remove_internal_shard branch December 11, 2014 16:31
@@ -418,7 +407,7 @@ public synchronized void removeShard(int shardId, String reason, @Nullable final
// and close the shard so no operations are allowed to it
if (indexShard != null) {
try {
((InternalIndexShard) indexShard).close(reason);
((IndexShard) indexShard).close(reason);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this cast?

s1monw added a commit that referenced this pull request Dec 11, 2014
We have lots of boilerplate code that is unnecessarily abstracting
services ie InternalIndexShard and IndexShard or InternalIndexService and
IndexService. It's enough to have concrete classes for these core classes.

Closes #8904

Conflicts:
	src/main/java/org/elasticsearch/action/deletebyquery/TransportShardDeleteByQueryAction.java
	src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java
	src/main/java/org/elasticsearch/index/IndexService.java
	src/main/java/org/elasticsearch/index/gateway/IndexShardGatewayService.java
	src/main/java/org/elasticsearch/index/gateway/local/LocalIndexShardGateway.java
	src/main/java/org/elasticsearch/index/percolator/PercolatorQueriesRegistry.java
	src/main/java/org/elasticsearch/index/service/IndexService.java
	src/main/java/org/elasticsearch/index/shard/IndexShard.java
	src/main/java/org/elasticsearch/index/shard/service/IndexShard.java
	src/main/java/org/elasticsearch/index/termvectors/ShardTermVectorService.java
	src/main/java/org/elasticsearch/indices/InternalIndicesService.java
	src/test/java/org/elasticsearch/common/lucene/docset/DocIdSetsTests.java
	src/test/java/org/elasticsearch/index/fielddata/AbstractFieldDataTests.java
	src/test/java/org/elasticsearch/index/mapper/string/SimpleStringMappingTests.java
	src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java
	src/test/java/org/elasticsearch/test/InternalTestCluster.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants