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

Added AutoCloseable interface to RiakClient #683

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

olitazl
Copy link

@olitazl olitazl commented Oct 31, 2016

I just added the AutoCloseable Interface to RiakClient to use the Try-with-Resource Statement.

*/
@Override
public void close(){
this.shutdown();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shutdown method returns Future, therefore to respect AutoCloseable it might be better to call as follows:

this.shutdown().get();

@olitazl
Copy link
Author

olitazl commented Oct 31, 2016

Good point! I added that!

@alexmoore
Copy link
Contributor

The RiakCluster / RiakClient objects are supposed to be longer-lived and manage a pool of connections that can be shared across multiple threads. Are you envisioning using this like JDBC connections?

@jplock
Copy link
Contributor

jplock commented Jan 1, 2017

Closeable extends from Autocloseable so I think we could just implement that - http://docs.oracle.com/javase/8/docs/api/java/io/Closeable.html

@olitazl
Copy link
Author

olitazl commented Jan 2, 2017

@alexmoore Not only but in some cases it makes it easier to implement small projects

@jplock could be a solution

@srgg
Copy link
Contributor

srgg commented Jan 3, 2017

I vote for AutoCloseable as it is more generic and modern interface, with the more generic contract.

But there is another issue, at the moment close() method is implemented as

public void close() throws ExecutionException, InterruptedException 

, whereas:

Implementers of this interface are also strongly advised to not have the close method throw InterruptedException. This exception interacts with a thread's interrupted status, and runtime misbehavior is likely to occur if an InterruptedException is suppressed. More generally, if it would cause problems for an exception to be suppressed, the AutoCloseable.close method should not throw it.

from the javadoc

@olitazl
Copy link
Author

olitazl commented Jan 3, 2017

@srgg hmm! Just catch it, doesn't seam enough! What kind of handling would you suggest?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants