-
Notifications
You must be signed in to change notification settings - Fork 11
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 etag support to system ComputationParticipant. #1654
Conversation
97680a6
to
50542a5
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 30 files at r1, all commit messages.
Reviewable status: 4 of 30 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/common/KingdomInternalException.kt
line 400 at r1 (raw file):
) : KingdomInternalException(ErrorCode.MEASUREMENT_ETAG_MISMATCH, message) { override val context get() = mapOf("actual_etag" to actualETag, "request_etag" to requestETag)
Is it possible to add the participant State into the context? So that the duchy can skip the rpc call if it is duplicated.
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: 4 of 30 files reviewed, 1 unresolved discussion (waiting on @renjiezh)
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/common/KingdomInternalException.kt
line 400 at r1 (raw file):
Previously, renjiezh wrote…
Is it possible to add the participant State into the context? So that the duchy can skip the rpc call if it is duplicated.
The context should just be the variables of the error message. It shouldn't be used to pass resource info. If the caller wants the state, it should do another read.
50542a5
to
aa968eb
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 26 of 30 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)
aa968eb
to
6c861e9
Compare
Depends on #1664 |
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 25 of 30 files at r1, 2 of 2 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/common/ETags.kt
line 22 at r3 (raw file):
import org.wfanet.measurement.gcloud.common.toInstant object ETags {
why this duplicate object? that just calls common?
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, 1 unresolved discussion (waiting on @stevenwarejones)
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/common/ETags.kt
line 22 at r3 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
why this duplicate object? that just calls common?
This version is for com.google.cloud.Timestamp
, which is currently only used for Spanner. The top-level common shouldn't have that dependency
6c861e9
to
c2849e0
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: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)
No description provided.