-
Notifications
You must be signed in to change notification settings - Fork 80
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
BarrageSubscriptionImpl: fix race conditions in subscription logic #4630
Conversation
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.
We need some further refactoring, which may simplify matters.
@@ -17,7 +17,7 @@ public class GrpcUtil { | |||
private static final Logger log = LoggerFactory.getLogger(GrpcUtil.class); | |||
|
|||
public static StatusRuntimeException securelyWrapError(final Logger log, final Throwable err) { | |||
return securelyWrapError(log, err, Code.INVALID_ARGUMENT); | |||
return securelyWrapError(log, err, Code.ABORTED); |
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.
My reading of the documentation for ABORTED suggests that this is not the best change to make. It also seems like a deviation from our norms. Maybe @niloc132 can weigh in.
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.
Cristian, Colin, and I dug in a bit and agree that ABORTED is an acceptable alternative. Per our discussion, Ryan, it would be better to change this in isolation swapping the status type for other similar cases.
server/src/main/java/io/deephaven/server/barrage/BarrageMessageProducer.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/deephaven/server/barrage/BarrageMessageProducer.java
Show resolved
Hide resolved
* | ||
* @return number of rows received by the subscription handler | ||
*/ | ||
long getRowsReceived(); |
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.
Check if Devin, Andy, or Charles are using this. I expect not.
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSubscriptionImpl.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSubscriptionImpl.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSubscriptionImpl.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSubscriptionImpl.java
Outdated
Show resolved
Hide resolved
extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageTable.java
Outdated
Show resolved
Hide resolved
extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageTable.java
Show resolved
Hide resolved
…eProducer.java Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
…rageSubscriptionImpl.java Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
…rageSubscriptionImpl.java Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
b7e0dd4
to
72d05b3
Compare
extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageTable.java
Outdated
Show resolved
Hide resolved
extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageTable.java
Outdated
Show resolved
Hide resolved
extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageTable.java
Outdated
Show resolved
Hide resolved
extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageTable.java
Show resolved
Hide resolved
extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageTable.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSubscriptionImpl.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSubscriptionImpl.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSubscriptionImpl.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSubscriptionImpl.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSubscriptionImpl.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageTable.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSnapshot.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSnapshot.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSnapshotImpl.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSnapshotImpl.java
Outdated
Show resolved
Hide resolved
java-client/barrage/src/main/java/io/deephaven/client/impl/BarrageSnapshotImpl.java
Outdated
Show resolved
Hide resolved
extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageTable.java
Show resolved
Hide resolved
Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
Fixes #803
Fixes #3300
Fixes #4594
The rnd4 (1:56pm MT) nightlies: https://github.com/nbauernfeind/deephaven-core/actions/runs/6552157706