Make Callbacks Easier to Use #3360
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pkoenig10
reviewed
Jul 9, 2018
|
|
||
| 1. Once all the resources for the transaction manager have become ready, **but before the transaction manager becomes initialized**, the registered callback will be run asynchronously. | ||
| 2. If the callback fails and should not be retried, the transaction manager will be closed. | ||
| 3. If the callback is successful, the transaction manager then becomes initialized. |
There was a problem hiding this comment.
Probably worth clarifying that the transaction manager provided to the callback can be used before the created transaction manager is initialized.
jeremyk-91
approved these changes
Jul 11, 2018
| @@ -40,7 +52,7 @@ | |||
| * @param initException Throwable thrown by init() | |||
| */ | |||
| public void cleanup(R resource, Throwable initException) { | |||
| Throwables.rewrapAndThrowUncheckedException(initException); | |||
| throw Throwables.rewrapAndThrowUncheckedException(initException); | |||
| * onInitFailureCleanup is called before calling initialize again. | ||
| * @param initialize initialization method. | ||
| * @param onInitFailureCleanup cleanup to be done in case initialization throws. If this method also throws, no more | ||
| * retries will be attampted and it will bubble up to the caller. |
| } | ||
| } | ||
|
|
||
| @Value.Modifiable |
There was a problem hiding this comment.
Interesting! Wasn't aware of this feature
There was a problem hiding this comment.
Yeah really useful, I saw it in one of the recent PR's
| } | ||
|
|
||
| @Test | ||
| public void singleAttemptCallbackCallsInitializeAndCleanuoOnceThenRethrowsOnInitFailure() { |
| @@ -0,0 +1,32 @@ | |||
| =========================== | |||
| Asynchronous Initialization | |||
| =========================== | |||
There was a problem hiding this comment.
Thanks for writing this! I don't think we documented this properly before (certainly not in such detail)
|
|
||
| /** | ||
| * Returns a Callback that will retry initialization on failure, unless the cleanup task also throws, in which case | ||
| * the cause is not caught. |
There was a problem hiding this comment.
Probably implied, but for clarity I'd say in which case the exception thrown by the cleanup task is propagated
There was a problem hiding this comment.
That's the word I was looking for!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goals (and why):
Make it easier to use Callbacks: make it useful, and prevent wrong usage. Address #3353
Implementation Description (bullets):
Added a bunch of docs, and added an interface that should make it simpler to create your callback.
Concerns (what feedback would you like?):
Is this actually simpler now? Do we need a better example -- I could have
ConsistencyCheckRunnerimplementCallbackInitializableinstead of being aCallback, but not sure it makes sense to do so just to have another example in the codebase.Where should we start reviewing?:
CallbackInitializable.javaPriority (whenever / two weeks / yesterday):
Not super urgent
@pkoenig10 for SA
This change is