as per @itaiy suggested will close then try to connect. by b-slim · Pull Request #3669 · apache/druid

b-slim · 2016-11-08T14:50:05Z

leventov · 2016-11-08T16:14:21Z

              Thread.currentThread().interrupt();
            } else if (e instanceof SocketException) {
+              pickledGraphite.close();
+              log.info("had exception [%s] trying to re-connect to graphite server", e.getMessage());


The exception is already logged a few lines above, maybe this logging statement should just tell "trying to re-connect.."?

leventov · 2016-11-08T16:15:09Z

@@ -175,11 +173,12 @@ public void run()
            if (e instanceof InterruptedException) {
              Thread.currentThread().interrupt();


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

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

@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.

oh i see you are right thanks for the catch.

leventov · 2016-11-08T16:18:28Z

            if (e instanceof InterruptedException) {
              Thread.currentThread().interrupt();
            } else if (e instanceof SocketException) {
+              pickledGraphite.close();


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.

b-slim · 2016-11-08T18:49:51Z

@leventov thanks for looking at this i have updated the pr

leventov

LGTM

itaiy · 2016-11-09T07:30:47Z

            }
          }
        }
-        pickledGraphite.flush();


@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())

b-slim · 2016-11-09T15:51:20Z

@itaiy close will do the flush as well.

itaiy · 2016-11-10T08:21:52Z

@b-slim - you mean GraphiteEmitter.close()?
I think it will still cause you to lose metrics, as you create a new PickledGraphite each time ConsumerRunnable.run() is invoked, so metrics left in the metrics' list inside the PickledGraphite won't be sent if PickledGraphite.flush() won't be called.

b-slim · 2016-11-10T17:54:20Z

    public void run()
    {
-      try {
+      try (PickledGraphite pickledGraphite = new PickledGraphite(


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

Right, I missed it, thanks :)

fjy · 2016-12-13T18:18:32Z

👍

nishantmonu51

👍

@itaiy

* as per @itaiy suggested will close then try to connect. * use close instead of flush * git fix comments * break the loop in case of interrupted

sata · 2017-02-20T17:13:35Z

Will this fix be backported to 0.9.2 branch? At the moment this fix is not included when pulling down the extension with pull-deps through tools.

fjy · 2017-02-20T17:16:23Z

@sata No, it will be a part of the upcoming 0.10.0 release.

sata · 2017-02-28T17:27:38Z

I tried this fix by backporting it myself and I'm finding the solution not to be working for the scenario I'm seeing.

HostedGraphite seems to have very aggressive timeouts in longer periods where they close their end roughly after a minute and a few seconds. Our interval window is sending each minute - if we have enough metrics to send.

The metric library which the extension uses, drops the metrics if it cannot write to the socket. If this happens regularly then we loose metrics. I have seen this happening for a full day on several occasions.

It seems this issue isn't new either[1].

I tried to leverage TCP keep alives on the socket[2] in conjunction with these kernel settings:

net.ipv4.tcp_keepalive_time=20
net.ipv4.tcp_keepalive_probes=3
net.ipv4.tcp_keepalive_intvl=5

And it didn't help. Since I don't know how HostedGraphite firewallls look like it's quite hard to reason about it. One thing didn't investigate is to see if our probes actually were acknowledge or not.

The approach I tried with first which did work but less elegant was to send a garbage byte as payload to the end point. [3].

This does the job but it isn't pretty. Preferably an application layer keep alive would be better but that would require a change to Carbon.

Has anyone else encountered this problem with HostedGraphite?

I would preferably avoid maintaining a fork if that's possible.

[1] dropwizard/metrics#669
[2] GameAnalytics/metrics@abedd44
[3] GameAnalytics/metrics@312e5b5

@itaiy

* as per @itaiy suggested will close then try to connect. * use close instead of flush * git fix comments * break the loop in case of interrupted

@itaiy

* as per @itaiy suggested will close then try to connect. * use close instead of flush * git fix comments * break the loop in case of interrupted

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

e0c9c4f

b-slim mentioned this pull request Nov 8, 2016

reconnect to the graphite after transient disconnect #2952

Merged

use close instead of flush

b8039b7

leventov reviewed Nov 8, 2016

View reviewed changes

git fix comments

84d4590

break the loop in case of interrupted

4c50d08

leventov reviewed Nov 8, 2016

View reviewed changes

itaiy reviewed Nov 9, 2016

View reviewed changes

b-slim commented Nov 10, 2016

View reviewed changes

b-slim added the Bug label Nov 11, 2016

b-slim added this to the 0.9.3 milestone Nov 11, 2016

gianm assigned nishantmonu51 and fjy Nov 29, 2016

nishantmonu51 approved these changes Dec 13, 2016

View reviewed changes

nishantmonu51 merged commit 7b18fb7 into apache:master Dec 13, 2016

b-slim deleted the fixing_reconnenct_graphite branch April 26, 2018 01:28

clambertus unassigned fjy and nishantmonu51 Jul 6, 2018

seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Feb 25, 2022

apache#3669 Port some more sql functions from apache druid

d7d319f

		@@ -175,11 +173,12 @@ public void run()
		if (e instanceof InterruptedException) {
		Thread.currentThread().interrupt();

Conversation

b-slim commented Nov 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-slim commented Nov 8, 2016

Uh oh!

leventov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-slim commented Nov 9, 2016

Uh oh!

itaiy commented Nov 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjy commented Dec 13, 2016

Uh oh!

nishantmonu51 left a comment

Choose a reason for hiding this comment

Uh oh!

sata commented Feb 20, 2017

Uh oh!

fjy commented Feb 20, 2017

Uh oh!

sata commented Feb 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

leventov Nov 8, 2016 •

edited

Loading