-
Notifications
You must be signed in to change notification settings - Fork 79
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
Plumb whereIn/whereNotIn through gRPC #3090
Plumb whereIn/whereNotIn through gRPC #3090
Conversation
Additionally, removes WhereNotInTable and collapses logic into WhereInTable parameters. Follow-up deephaven#3088 Fixes deephaven#990
final List<JoinMatch> columnsToMatch = JoinMatch.from(request.getColumnsToMatchList()); | ||
final AwareFunctionalLock lock = | ||
left.isRefreshing() || right.isRefreshing() ? updateGraphProcessor.sharedLock() : null; | ||
if (lock != null) { |
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.
why explicit lock+unlock over computeLocked? at least as a reader the computeLocked is more concise, more obviously correct ("any chance that something could throw on the way out of lock() that would leave it locked, should we just do the lock inside the try/catch").
Example
io.deephaven.server.table.ops.JoinTablesGrpcImpl#create
final Table result;
if (!lhs.isRefreshing() && !rhs.isRefreshing()) {
result = realTableOperation.apply(lhs, rhs, columnsToMatch, columnsToAdd, request);
} else {
result = updateGraphProcessor.sharedLock().computeLocked(
() -> realTableOperation.apply(lhs, rhs, columnsToMatch, columnsToAdd, request));
}
return result;
(could just as easily return from inside the if/else)
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'm not a fan of lambda abstractions for singular operations (I see where the abstraction comes in handy for join where we have a lot of them), and the branching that results around the application. That said, the version I has has branching logic around the lock... something I personally prefer, but I think we can do better...
@rcaudy @niloc132 , would you be opposed to adding a new method to FunctionalLock:
public interface FunctionalLock extends Lock {
...
default SafeCloseable closeableLock() {
final SafeCloseableLock closeable = new SafeCloseableLock(this);
lock();
return closeable;
}
This would allow the use of a try resource, which I think is much more ergonomic:
@Override
public final Table create(final WhereInRequest request, final List<SessionState.ExportObject<Table>> sourceTables) {
...
try (final SafeCloseable _lock = lock(left, right)) {
return request.getInverted() ? left.whereNotIn(right, columnsToMatch) : left.whereIn(right, columnsToMatch);
}
}
private SafeCloseable lock(Table left, Table right) {
return left.isRefreshing() || right.isRefreshing() ? updateGraphProcessor.sharedLock().closeableLock() : null;
}
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 don't hate it. Not a bad option, for cases where lambdas are annoying.
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.
One minor remark, discussed in slack also about getting js api bindings closer to working
final Table left = sourceTables.get(0).get(); | ||
final Table right = sourceTables.get(1).get(); | ||
final List<JoinMatch> columnsToMatch = JoinMatch.from(request.getColumnsToMatchList()); | ||
try (final SafeCloseable _lock = lock(left, right)) { |
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.
why a new idiom here, when we have one already?
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.
See commentary earlier for color - it's more concise and easier to read (and write) IMO.
Additionally, removes WhereNotInTable and collapses logic into WhereInTable parameters.
Follow-up #3088
Fixes #990