-
Notifications
You must be signed in to change notification settings - Fork 271
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
Add a transactional parameter to @Action #2133
base: master
Are you sure you want to change the base?
Conversation
b2c7d3d
to
788fdd8
Compare
788fdd8
to
7ab6511
Compare
return; | ||
} | ||
|
||
tm().transact( |
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.
I really wish there were a way to make these 3 repeated calls nicer, but I couldn't find any. You can't extract to a helper function that takes a lambda.
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.
It seems like most of the change are to various commands. Maybe you should consider changing the title of the PR to reflect that.
Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys)
core/src/main/java/google/registry/request/Action.java
line 63 at r1 (raw file):
/** Whether to create a wrapping DB transaction around the execution of the action. */ boolean transactional() default true;
Are we sure we want to set the default to true? Did we survey the actions and conclude that the majority of them need to be wrapped in a transaction in their entirety?
core/src/main/java/google/registry/tools/ConfirmingCommand.java
line 38 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
I really wish there were a way to make these 3 repeated calls nicer, but I couldn't find any. You can't extract to a helper function that takes a lambda.
The root problem is that tm().transact()
expects a Runnable
, whose run()
method doesn't declare a checked exception, so your lambda cannot either.
You can get around it by making your lambda a Callable<Void>
, then feeding it to a wrapper method that converts it back to a Runnable
but rethrows the checked exception as unchecked. The key here is that the Callable.call()
method declares a checked exception, so you are allowed to supply a lambda that throws a checked exception.
So it would be like this:
private static Runnable wrapper(Callable<Void> callable) {
return new Runnable() {
@Override
public void run() {
try {
callable.call();
} catch (Exception e) {
rethrow(e);
}
}
};
}
tm().transact(wrapper(
() -> {
init();
printLineIfNotEmpty(prompt());
return null;
}));
Whether it is preferable to just do the rethrow manually is up for debate.
Alternatively, I see no reason why we cannot just have tm().transact()
take a Callable<T>
instead of a Supplier<T>
(which doesn't allow checked exceptions), so you don't need a wrapper method and can just do:
tm().transact(
() -> {
init();
printLineIfNotEmpty(prompt());
return null;
});
We can maybe go a step further and define our own ThrowableRunnable
that allows throwing, and make tm().transact()
take it instead of the vanilla Runnable
, in which case you can just provide the lambda as is.
It seems to me that there really isn't a need to force the caller to handle checked exceptions when supplying a lambda to tm().transact()
, if all they do is to just re-throw them as unchecked. tm().transact()
wraps the work in its own try block so any exception thrown by the workload would be handled by the transaction manager anyway.
core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java
line 221 at r1 (raw file):
private Map<String, Object> read(String registrarId) { return tm().transact(
What's wrong with just wrapping loadRegistrarUnchecked(registrarId)
inside the transaction? That way you don't need to change the return value here.
Also, the entire RegistrarResult
class seems redundant at this point, since you are just calling the toJsonResponse()
method on it to get the map anyway.
core/src/test/java/google/registry/whois/WhoisCommandFactoryTest.java
line 100 at r1 (raw file):
() -> { WhoisResponse response = assertDoesNotThrow(
Why do we need this assertion? If the lambda throws, wouldn't the whole test method fail anyway?
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.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jianglai)
core/src/main/java/google/registry/request/Action.java
line 63 at r1 (raw file):
Previously, jianglai (Lai Jiang) wrote…
Are we sure we want to set the default to true? Did we survey the actions and conclude that the majority of them need to be wrapped in a transaction in their entirety?
Yes, I did survey the actions and determine that almost all of them are transactional.
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.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jianglai)
core/src/test/java/google/registry/whois/WhoisCommandFactoryTest.java
line 100 at r1 (raw file):
Previously, jianglai (Lai Jiang) wrote…
Why do we need this assertion? If the lambda throws, wouldn't the whole test method fail anyway?
If the lambda throws then it can't be passed to the lambda in transact()
. The checked exception needs to be handled. assertDoesNotThrow()
handles the checked exception and does not rethrow a checked exception.
7ab6511
to
61eb640
Compare
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.
Reviewable status: 10 of 15 files reviewed, 4 unresolved discussions (waiting on @jianglai)
core/src/main/java/google/registry/tools/ConfirmingCommand.java
line 38 at r1 (raw file):
Previously, jianglai (Lai Jiang) wrote…
The root problem is that
tm().transact()
expects aRunnable
, whoserun()
method doesn't declare a checked exception, so your lambda cannot either.You can get around it by making your lambda a
Callable<Void>
, then feeding it to a wrapper method that converts it back to aRunnable
but rethrows the checked exception as unchecked. The key here is that theCallable.call()
method declares a checked exception, so you are allowed to supply a lambda that throws a checked exception.So it would be like this:
private static Runnable wrapper(Callable<Void> callable) { return new Runnable() { @Override public void run() { try { callable.call(); } catch (Exception e) { rethrow(e); } } }; } tm().transact(wrapper( () -> { init(); printLineIfNotEmpty(prompt()); return null; }));
Whether it is preferable to just do the rethrow manually is up for debate.
Alternatively, I see no reason why we cannot just have
tm().transact()
take aCallable<T>
instead of aSupplier<T>
(which doesn't allow checked exceptions), so you don't need a wrapper method and can just do:tm().transact( () -> { init(); printLineIfNotEmpty(prompt()); return null; });
We can maybe go a step further and define our own
ThrowableRunnable
that allows throwing, and maketm().transact()
take it instead of the vanillaRunnable
, in which case you can just provide the lambda as is.It seems to me that there really isn't a need to force the caller to handle checked exceptions when supplying a lambda to
tm().transact()
, if all they do is to just re-throw them as unchecked.tm().transact()
wraps the work in its own try block so any exception thrown by the workload would be handled by the transaction manager anyway.
Thanks for your ThrowingRunnable
PR, this is nicer now.
61eb640
to
9bd452d
Compare
This also removes inner transactions on some Registrar methods with some resultant refactoring.
9bd452d
to
045655d
Compare
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.
Reviewed 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys)
core/src/main/java/google/registry/request/Action.java
line 63 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Yes, I did survey the actions and determine that almost all of them are transactional.
See my other comment below, EppTlsAction
needs to be marked as non-transactional, and there needs to be another field to specify the transaction isolation level override, if any.
core/src/main/java/google/registry/request/RequestHandler.java
line 161 at r3 (raw file):
Runnable action = route.get().instantiator().apply(component); if (route.get().action().transactional()) { tm().transact(action::run);
Now that we have a good understanding on how to handle nested transactions, I have some concern on how this would work together with the transaction isolation level override in the long run.
The override can only be applied at the top level transaction, which would be here. We therefore need to allow setting the override (possibly via another field in the@Action
annotation).
However, all EPP flows are executed by EppTlsAction
, and we have envisioned using the @IsolationLevel
annotation on a flow to signal to the FlowRunner
.
In order to allow different EPP flows to use different isolation levels, we need to annotate the EppTlsAction
as non-transactional, and define the transaction behavior at the flow level. This is doable, but needs to be clearly documented to avoid confusion.
core/src/main/java/google/registry/tools/ConfirmingCommand.java
line 38 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Thanks for your
ThrowingRunnable
PR, this is nicer now.
It's it sooooo much nicer!
core/src/test/java/google/registry/whois/WhoisCommandFactoryTest.java
line 100 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
If the lambda throws then it can't be passed to the lambda in
transact()
. The checked exception needs to be handled.assertDoesNotThrow()
handles the checked exception and does not rethrow a checked exception.
Does it still apply now that we can throw checked exceptions in the lambda supplied to tm().transact()
? It will just be wrapped in a RuntimeException
if it throws, correct?
This starts the process of removing inner transactions on some Registrar methods with some resultant refactoring, but then I gave up after awhile and turned them into reTransact() calls just to be able to get in the Action changes quickly.
This change is