Skip to content
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

Implement internal spanner stream exchanges method #975

Merged

Conversation

jcorilla
Copy link
Contributor

@jcorilla jcorilla commented May 1, 2023

No description provided.

@wfa-reviewable
Copy link

This change is Reviewable

@jcorilla jcorilla force-pushed the jcorilla-exchange-stream-spanner-internal-implementation branch 2 times, most recently from c7cdd72 to df6a540 Compare May 4, 2023 18:31
@jcorilla jcorilla requested a review from tristanvuong2021 May 4, 2023 18:42
@jcorilla jcorilla marked this pull request as ready for review May 4, 2023 18:42
@jcorilla jcorilla force-pushed the jcorilla-exchange-stream-spanner-internal-implementation branch from df6a540 to e2aa808 Compare May 9, 2023 22:18
Copy link
Contributor

@tristanvuong2021 tristanvuong2021 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jcorilla)


src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/ExchangesServiceTest.kt line 283 at r3 (raw file):

  @Test
  fun `streamExchange respects limit`(): Unit = runBlocking {
    val createRequest1 = CreateExchangeRequest.newBuilder().apply { exchange = EXCHANGE }.build()

Switch to the builder used elsewhere instead of newBuilder.

Copy link
Contributor Author

@jcorilla jcorilla left a 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 5 files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)


src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/ExchangesServiceTest.kt line 283 at r3 (raw file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

Switch to the builder used elsewhere instead of newBuilder.

Done.

Copy link
Contributor

@tristanvuong2021 tristanvuong2021 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jcorilla)

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r2, 3 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier)

Copy link
Contributor

@Marco-Premier Marco-Premier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r2, 3 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jcorilla)

@jcorilla jcorilla force-pushed the jcorilla-exchange-stream-spanner-internal-implementation branch from 2993570 to 02d71b3 Compare May 11, 2023 16:00
Copy link
Contributor Author

@jcorilla jcorilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r2, 3 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jcorilla)

@jcorilla jcorilla merged commit abdd7d3 into main May 11, 2023
@jcorilla jcorilla deleted the jcorilla-exchange-stream-spanner-internal-implementation branch May 11, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants