-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix emit wait time #2869
fix emit wait time #2869
Conversation
@gianm checkout this |
@@ -24,7 +24,8 @@ All the configuration parameters for graphite emitter are under `druid.emitter.g | |||
|`druid.emitter.graphite.flushPeriod` | Queue flushing period in milliseconds. |no|1 minute| | |||
|`druid.emitter.graphite.maxQueueSize`| Maximum size of the queue used to buffer events. |no|`MAX_INT`| | |||
|`druid.emitter.graphite.alertEmitters`| List of emitters where alerts will be forwarded to. |no| empty list (no forwarding)| | |||
|`druid.emitter.graphite.emitWaitTime` | wait time in milliseconds to try to send the event otherwise emitter will throwing event. |no|1000 (1 sec)| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the default should be 0, it's rare that people would want to block their query threads waiting for metrics emission rather than just drop some metrics and have queries return faster.
@gianm fixed |
I still think the default for |
The rest generally looks good though |
@gianm make sense, fixed |
"Throwing event [%s] on the floor Graphite queue is full please increase the capacity or/and the consumer frequency", | ||
mapper.writeValueAsString(event) | ||
); | ||
countLostEvents.getAndIncrement(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this increment isn't necessary, it's happening again on the next line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch !
👍 lgtm |
Running io.druid.query.extraction.namespace.TestKafkaExtractionCluster |
Fix for #2868