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

Jump to bottom

Remove unnecessary intermediate interfaces #6517

Merged
merged 1 commit into from Jun 17, 2014
Merged

Remove unnecessary intermediate interfaces #6517

merged 1 commit into from Jun 17, 2014

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Jun 16, 2014

Client, ClusterAdminClient and IndicesAdminClient had corresponding intermediate internal interfaces that are unnecessary and cause a lot of casting. This commit removes the intermediate interfaces and uses the super interfaces directly.

This PR also simplifys FilterClient by subclassing AbstractClient

Closes #4355

javanna
javanna reviewed Jun 16, 2014
View reviewed changes
src/test/java/org/elasticsearch/test/client/FilterClient.java
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.threadpool.ThreadPool;

/**
*/
public abstract class FilterClient implements InternalClient {
Copy link
Member

Choose a reason for hiding this comment

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

was wondering if it makes sense to move this base class outside of test as I could use it in #6513 as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 I think you should do that in #6513

Copy link
Member

Choose a reason for hiding this comment

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

yep makes sense

@javanna
Copy link
Member

javanna commented Jun 16, 2014

LGTM, very nice improvement

@uboness
Copy link
Contributor

uboness commented Jun 16, 2014

can we also consider having one base interface for all clients... the all have the same execute methods (only the request types is different). If we do this, it'll also clean up #6513 as we'll only need a single copy headers filter client (I think)

@s1monw
Copy link
Contributor Author

s1monw commented Jun 16, 2014

@uboness I am working on makeing things more generic as we speak I think having a single execute method would make sense. Lemme see how it goes

@uboness
Copy link
Contributor

uboness commented Jun 16, 2014

awesome! thx

@s1monw
Copy link
Contributor Author

s1monw commented Jun 17, 2014

@uboness here are some updates I think we are close?

javanna
javanna reviewed Jun 17, 2014
View reviewed changes
src/main/java/org/elasticsearch/client/Client.java
* Closes the client.
*/
void close();
public interface Client extends ElasticsearchClient<Client>, Closeable {
Copy link
Member

Choose a reason for hiding this comment

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

Can you mention issue #4355 in the commit that makes Client implement Closeable? Also we might want to do the same with Node.

@jpountz
Copy link
Contributor

jpountz commented Jun 17, 2014

This looks great. We might want to make clients extend Releasable instead of Closeable to not have the IOException to deal with?

@uboness
Copy link
Contributor

uboness commented Jun 17, 2014

I LOVE IT!!! left a small comment on naming... but other than that LGTM

@s1monw
Copy link
Contributor Author

s1monw commented Jun 17, 2014

I pushed the fixes for the comments and added some more javadocs. I think it's ready! @javanna I will include the issue in the final commit message

javanna
javanna reviewed Jun 17, 2014
View reviewed changes
src/main/java/org/elasticsearch/client/FilterClient.java
* Util method that casts to the client base class for use inside the exectue methods only.
* This method allows to provide yet another client that is used to execute the actual remote calls.
*/
protected ElasticsearchClient executor() {
Copy link
Member

Choose a reason for hiding this comment

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

aren't we missing a generic type here? ElasticsearchClient<Client>

@javanna
Copy link
Member

javanna commented Jun 17, 2014

I left a few comments on generics types missing, other than that LGTM!!!

@s1monw
[CLIENT] Remove unnecessary intermediate interfaces
adb5c19 
Client, ClusterAdminClient and IndicesAdminClient had corresponding
intermediate `internal` interfaces that are unnecessary and cause
a lot of casting. This commit removes the intermediate interfaces
and uses the super interfaces directly.

This commit also adds Releaseable to `Node` and `Client` in order to
be used with utilities like try / with.

Closes elastic#4355
Closes elastic#6517
@s1monw s1monw merged commit adb5c19 into elastic:master Jun 17, 2014
@s1monw s1monw deleted the client_cleanup branch June 17, 2014 10:28
@s1monw
Copy link
Contributor Author

s1monw commented Jun 17, 2014

I mark this as breaking since it changes class hierarchies... yet, I don't think users will be affected since they only use the final classes rather than internal once... thanks for the reviews...

s1monw added a commit that referenced this pull request Jun 17, 2014
@s1monw
[CLIENT] Remove unnecessary intermediate interfaces
a46652b 
Client, ClusterAdminClient and IndicesAdminClient had corresponding
intermediate `internal` interfaces that are unnecessary and cause
a lot of casting. This commit removes the intermediate interfaces
and uses the super interfaces directly.

This commit also adds Releaseable to `Node` and `Client` in order to
be used with utilities like try / with.

Closes #4355
Closes #6517
@javanna javanna mentioned this pull request Jun 17, 2014
Closed
@javanna javanna mentioned this pull request Jun 19, 2014
Closed
javanna added a commit to javanna/elasticsearch that referenced this pull request Jun 19, 2014
@javanna
Client intermediate interface removal follow-up
edf40fd 
After elastic#6517 we ended up registering all of the actions (included admin ones) to the NodeClient.
Made sure that only the proper type of Action instances are registered to each client type.
Also fixed some compiler warnings: unused members, imports and non matching generic types.

Closes elastic#6563
javanna added a commit that referenced this pull request Jun 19, 2014
@javanna
Client intermediate interface removal follow-up
8f8b2d7 
After #6517 we ended up registering all of the actions (included admin ones) to the NodeClient.
Made sure that only the proper type of Action instances are registered to each client type.
Also fixed some compiler warnings: unused members, imports and non matching generic types.

Closes #6563
javanna added a commit that referenced this pull request Jun 19, 2014
@javanna
Client intermediate interface removal follow-up
505fac2 
After #6517 we ended up registering all of the actions (included admin ones) to the NodeClient.
Made sure that only the proper type of Action instances are registered to each client type.
Also fixed some compiler warnings: unused members, imports and non matching generic types.

Closes #6563
@clintongormley clintongormley changed the title [CLIENT] Remove unnecessary intermediate interfaces Java API: Remove unnecessary intermediate interfaces Jul 16, 2014
@clintongormley clintongormley added the :Core/Infra/Transport API Transport client API label Jun 6, 2015
@clintongormley clintongormley changed the title Java API: Remove unnecessary intermediate interfaces Remove unnecessary intermediate interfaces Jun 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
>breaking :Core/Infra/Transport API Transport client API >enhancement v1.3.0 v2.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Internal: Make Node and Client interfaces Closeable
6 participants
@s1monw @javanna @uboness @jpountz @dweiss @clintongormley