-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
[FLINK-8670] Make MetricRegistryImpl#shutdown non blocking #5504
Conversation
FutureUtils#completeAll(Collection) takes a collection of futures and returns a future which is completed after all of the given futures are completed. This also includes exceptional completions. Potentially occurring exceptions are recorded and combined into a single exception with which the resulting future is completed. FutureUtils#runAfterwards takes a future and runs a given action after the completion of the given future. This also includes an exceptional completion. In this case, a potentially occurring exception as the result of the provided action will be combined with the future's exception.
This commit makes the MetricRegistryImpl#shutdown method non blocking. Instead of waiting for the completion of the shutdown procedure, the method returns a future which is completed once the metric registry has completed the shut down.
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.
Just did a quick skim and no full review.
|
||
/** | ||
* Creates a new MetricRegistry and starts the configured reporter. | ||
*/ | ||
public MetricRegistryImpl(MetricRegistryConfiguration config) { | ||
this.scopeFormats = config.getScopeFormats(); | ||
this.globalDelimiter = config.getDelimiter(); | ||
this.delimiters = new ArrayList<>(10); |
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.
the number of delimiters is always equal to the number of reporters, so they should be initialized with the same size.
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 point, will change it.
*/ | ||
public void shutdown() { | ||
public CompletableFuture<Void> shutdown() { |
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.
How about renaming this to shutdownAsync? (and possible keep shutdown() that just calls shutdownAsync().get()).
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.
Hmm, I actually would like to avoid that people can introduce blocking operations into the ClusterEntrypoint
, for example. By offering a blocking shutdown
method, I fear that this can easily happen again.
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.
Fair point. What about the renaming though?
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.
Will do.
This commit makes the MetricRegistryImpl#shutdown method non blocking. Instead of waiting for the completion of the shutdown procedure, the method returns a future which is completed once the metric registry has completed the shut down. This closes apache#5504.
This commit makes the MetricRegistryImpl#shutdown method non blocking. Instead of waiting for the completion of the shutdown procedure, the method returns a future which is completed once the metric registry has completed the shut down. This closes apache#5504.
This commit makes the MetricRegistryImpl#shutdown method non blocking. Instead of waiting for the completion of the shutdown procedure, the method returns a future which is completed once the metric registry has completed the shut down. This closes apache#5504.
What is the purpose of the change
This commit makes the MetricRegistryImpl#shutdown method non blocking. Instead
of waiting for the completion of the shutdown procedure, the method returns a
future which is completed once the metric registry has completed the shut down.
This PR is based on #5503.
Verifying this change
This change is already covered by existing tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation