Skip to content

as per @itaiy suggested will close then try to connect. #3669

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.

Sign up for GitHub

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

Merged
nishantmonu51 merged 4 commits into from
Dec 13, 2016
Merged

as per @itaiy suggested will close then try to connect. #3669

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e0c9c4f
as per @itaiy suggested will close then try to connect.
b-slim Nov 8, 2016
b8039b7
use close instead of flush
b-slim Nov 8, 2016
84d4590
git fix comments
b-slim Nov 8, 2016
4c50d08
break the loop in case of interrupted
b-slim Nov 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions ...ons-contrib/graphite-emitter/src/main/java/io/druid/emitter/graphite/GraphiteEmitter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,16 +136,14 @@ public void emit(Event event)

private class ConsumerRunnable implements Runnable
{
private PickledGraphite pickledGraphite = new PickledGraphite(
graphiteEmitterConfig.getHostname(),
graphiteEmitterConfig.getPort(),
graphiteEmitterConfig.getBatchSize()
);

@Override
public void run()
{
try {
try (PickledGraphite pickledGraphite = new PickledGraphite(
Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Nov 10, 2016

Choose a reason for hiding this comment

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

@itaiy this try will call the close once run is done.

Copy link
Copy Markdown

@itaiy itaiy Nov 13, 2016

Choose a reason for hiding this comment

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

Right, I missed it, thanks :)

graphiteEmitterConfig.getHostname(),
graphiteEmitterConfig.getPort(),
graphiteEmitterConfig.getBatchSize()
)) {
if (!pickledGraphite.isConnected()) {
log.info("trying to connect to graphite server");
pickledGraphite.connect();
Expand Down Expand Up @@ -174,12 +172,16 @@ public void run()
log.error(e, e.getMessage());
if (e instanceof InterruptedException) {
Thread.currentThread().interrupt();
Copy link
Copy Markdown
Member

@leventov leventov Nov 8, 2016

Choose a reason for hiding this comment

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

What practically happens after this statement? We continue with the next iteration in the loop?

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Nov 8, 2016

Choose a reason for hiding this comment

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

if it is interrupted it will terminate the current thread right ?

Copy link
Copy Markdown
Member

@leventov leventov Nov 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@b-slim as far as I know it will just set interrupted flag and continue executing as normal: https://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#interrupt() The next blocking operation may fail immediately with InterruptedException.

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Nov 8, 2016

Choose a reason for hiding this comment

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

oh i see you are right thanks for the catch.

break;
} else if (e instanceof SocketException) {
// This is antagonistic to general Closeable contract in Java,
// it is needed to allow re-connection in case of the socket is closed due long period of inactivity
pickledGraphite.close();
Copy link
Copy Markdown
Member

@leventov leventov Nov 8, 2016

Choose a reason for hiding this comment

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

General Closeable contract in Java is that after close() the object is useless: either doesn't react to any subsequent calls or throws runtime exceptions. If this is not so for PickledGraphite, maybe it should be commented.

Copy link
Copy Markdown
Contributor Author

@b-slim b-slim Nov 8, 2016

Choose a reason for hiding this comment

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

ok

log.warn("Trying to re-connect to graphite server");
pickledGraphite.connect();
}
Copy link
Copy Markdown

@itaiy itaiy Nov 9, 2016

Choose a reason for hiding this comment

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

@b-slim - why did you remove flush()?
I might be wrong, but seems to me that if you have some metrics in pickledGraphite that weren't written to the OutputStream in the while loop, they will never be sent (as pickledGraphite.send() only writes to the OutputStream when the size of the metrics' list is equal or greater than the batch size, see PickledGraphite.send())

}
}
pickledGraphite.flush();
}
catch (Exception e) {
log.error(e, e.getMessage());
Expand Down