Repository navigation
#165: Make YdbValidator implementation customizable via YdbRepository settings - #238
nvamelichev wants to merge 1 commit into
Conversation
Files
|
e2e84dc to
507f414
Compare
Files
|
507f414 to
d680458
Compare
Files
|
d680458 to
8c05118
Compare
Files
|
8c05118 to
0547c6f
Compare
Files
|
0547c6f to
9cd9918
Compare
Files
|
|
@lavrukov Gentle ping: Please take a look at this PR, I'd like to include it in the next release of YOJ ;-) |
9cd9918 to
299adc1
Compare
Files
|
299adc1 to
9335861
Compare
Files
|
…ory.Settings` This opens the door for error handling customization that can work without changing the core error handling logic in `repository-ydb-v2`.
9335861 to
d13cf4e
Compare
Files
|
| } else if (ex instanceof TimeoutException) { | ||
| return convertToUnavailable(ex); | ||
| } else if (ex instanceof ExecutionException) { | ||
| if (ex.getCause() instanceof RepositoryException re) { |
There was a problem hiding this comment.
just interestign - what problem solves this if ?
There was a problem hiding this comment.
scan query used to throw a mostly useless-for-error-handling RepositoryException("Caught ExecutionException", cause=ResultTruncatedException("...")); now we throw ResultTruncatedException directly. See the updated RepositoryTest for the difference.
| * Validates that {@code status} is successful, and throws an appropriate subtype of {@link RepositoryException} | ||
| * if {@code status} is an error. | ||
| * | ||
| * @param request Information about YDB request being made. You may inspect its {@link DbRequest#getType()} type}, |
There was a problem hiding this comment.
Reading this 14-line comment, I don’t think it adds much beyond what the method signature already tells us. I’d keep the first two lines at most and remove the rest.
In general, I’d prefer fewer comments. I think comments should be short and explain decisions that aren’t obvious from the code. For everything else, I’d aim to make the code clear enough to speak for itself.
I get that this is a common Java style, but it feels very verbose, and I’m worried we’ll get lost in all these comments.
| * <li>{@link #GET_SESSION GET_SESSION - getSession()}</li> | ||
| * </ul> | ||
| */ | ||
| @Value |
There was a problem hiding this comment.
I've started with a record but that didn't give me enough flexibility to have a private all-args constructor to prevent people from constructing their own e.g. commit operations with a non-empty args list
| */ | ||
| public static final DbRequest GET_SESSION = new DbRequest(Type.GET_SESSION); | ||
|
|
||
| private DbRequest(@NonNull Type type) { |
There was a problem hiding this comment.
issue: I think it would be better to inline this: GET_SESSION = new DbRequest(Type.GET_SESSION, List.of());
This constructor only saves a few characters, but the call doesn’t make it clear what args contains. You have to check the constructor implementation and then come back. Inlining it would save that extra step.
| } | ||
|
|
||
| public static DbRequest dataQuery(@Nullable String log) { | ||
| return new DbRequest(Type.DATA_QUERY, singletonList(log)); |
There was a problem hiding this comment.
nit: singletonList won’t give us a performance benefit here, and we use List.of() everywhere else. I find the inconsistency a bit confusing, so I’d use List.of() throughout.
There was a problem hiding this comment.
log might genuinely be null (because Statement.toDebugParams(...) currently may return null and actually does this in RepositoryTest), and List.of() doesn't permit null elements.
That's why we use singletonList() here and for scan query. And that's a minor implementation detail, which we'll likely fix when we overhaul statement logging (for which I have a few long-waiting PRs ;-) )
| SCAN_QUERY { | ||
| @Override | ||
| public String formatPrefix() { | ||
| return "scan()."; |
There was a problem hiding this comment.
Maybe we could use a new, consistent approach here instead of repeating the old logic? Let’s have everything go through .scanQuery(), .dataQuery(), .readTable(), and .rollback().
It seems like this would simplify the code without losing any information in the logs. What do you think?
|
|
||
| @NonNull | ||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
Something feels a bit off here. I tried using a sealed interface, and I think the result looks cleaner: https://pastebin.com/9CFfxzRy
| @InternalApi | ||
| @RequiredArgsConstructor(access = PRIVATE) | ||
| @ExperimentalApi(issue = "https://github.com/ydb-platform/yoj-project/issues/165") | ||
| public class DbRequest { |
There was a problem hiding this comment.
The name feels too generic. Maybe DbRequestLog or something similar would make it clearer that this is only used for logging?
This opens the door for error handling customization that can work without changing the core error handling logic in
repository-ydb-v2.