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

Update cross-media-measurement-api version #1123

Merged
merged 6 commits into from
Aug 1, 2023

Conversation

Marco-Premier
Copy link
Contributor

Replace Timestamps with google.type.Date in ModelRolloutService Fix broken tests

Internal proto resources remained the same. What changed is the way public proto resources are converted into internal ones and viceversa.

Replace Timestamps with google.type.Date in ModelRolloutService
Fix broken tests
@wfa-reviewable
Copy link

This change is Reviewable

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 5 of 10 files at r1, all commit messages.
Reviewable status: 5 of 10 files reviewed, 1 unresolved discussion (waiting on @jojijac0b and @Marco-Premier)


src/main/kotlin/org/wfanet/measurement/api/v2alpha/tools/MeasurementSystem.kt line 1781 at r1 (raw file):

      required = false,
    )
    rolloutStartDate: Instant? = null,

Picocli supports LocalDate, which is the Java equivalent of the google.type.Date protobuf message type.

Suggestion:

LocalDate

Copy link
Contributor Author

@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.

Reviewable status: 4 of 12 files reviewed, 1 unresolved discussion (waiting on @jojijac0b and @SanjayVas)


src/main/kotlin/org/wfanet/measurement/api/v2alpha/tools/MeasurementSystem.kt line 1781 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Picocli supports LocalDate, which is the Java equivalent of the google.type.Date protobuf message type.

Done.

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 all commit messages.
Reviewable status: 4 of 12 files reviewed, 1 unresolved discussion (waiting on @jojijac0b and @Marco-Premier)


src/main/kotlin/org/wfanet/measurement/api/v2alpha/tools/MeasurementSystem.kt line 1816 at r2 (raw file):

            month = instantRolloutDate.monthValue
            day = instantRolloutDate.dayOfMonth
          }

Use org.wfanet.measurement.common.toProtoDate extension

Suggestion:

          this.instantRolloutDate = instantRolloutDate.toProtoDate()

Copy link
Contributor Author

@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.

Reviewable status: 4 of 12 files reviewed, 1 unresolved discussion (waiting on @jojijac0b and @SanjayVas)


src/main/kotlin/org/wfanet/measurement/api/v2alpha/tools/MeasurementSystem.kt line 1816 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Use org.wfanet.measurement.common.toProtoDate extension

Done.

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 10 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: 6 of 12 files reviewed, 3 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)


src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ModelRolloutsService.kt line 382 at r3 (raw file):

      month = localDate.monthValue
      day = localDate.dayOfMonth
    }

You can chain the existing conversion methods. You may want to do this explicitly rather than hiding the implementation in an extension method.

Suggestion:

    return toInstant().atZone(ZoneOffset.UTC).toLocalDate().toProtoDate()

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ModelRolloutsService.kt line 392 at r3 (raw file):

      seconds = instant.epochSecond
      nanos = instant.nano
    }

You can chain the existing conversion methods. You may want to do this explicitly rather than hiding the implementation in an extension method, or at least being explicit in the method name that this is converting to start of day UTC.

Suggestion:

   return toLocalDate().atStartOfDay().toInstant(ZoneOffset.UTC).toProtoTime()

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 928 at r3 (raw file):

}

// TODO(@MarcoPremier): Move this function to common-jvm.

I believe you have these defined in 4 locations. Even if you want them and intend to move to common-jvm later, at least put them in some common location in this repo now.

Copy link
Contributor

@jojijac0b jojijac0b left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 10 files at r1, 3 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Marco-Premier)

Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 10 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Marco-Premier)


src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ModelRolloutsService.kt line 309 at r3 (raw file):

              source.filter.hasRolloutPeriodOverlapping() &&
              source.filter.rolloutPeriodOverlapping.startDate ==
                this.rolloutPeriodOverlapping.startTime.toDate() &&

feels funny to update only some of these from times to date. The latest cm-api as rolloutPeriodOverlapping.startDate

Copy link
Contributor Author

@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.

@SanjayVas I've avoided to update common-jvm in favor of concatenation of existing methods. thanks

Reviewable status: 9 of 12 files reviewed, 4 unresolved discussions (waiting on @jojijac0b, @SanjayVas, and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ModelRolloutsService.kt line 309 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

feels funny to update only some of these from times to date. The latest cm-api as rolloutPeriodOverlapping.startDate

That's because underlying startDate / endDate management in internal APIs did not change. So the service take care of converting dates to prototimes before forwarding requests internally


src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ModelRolloutsService.kt line 382 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

You can chain the existing conversion methods. You may want to do this explicitly rather than hiding the implementation in an extension method.

Done.


src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ModelRolloutsService.kt line 392 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

You can chain the existing conversion methods. You may want to do this explicitly rather than hiding the implementation in an extension method, or at least being explicit in the method name that this is converting to start of day UTC.

Done.


src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 928 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I believe you have these defined in 4 locations. Even if you want them and intend to move to common-jvm later, at least put them in some common location in this repo now.

Done.

Copy link
Collaborator

@stevenwarejones stevenwarejones 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 4 files at r2, all commit messages.
Reviewable status: 9 of 12 files reviewed, 4 unresolved discussions (waiting on @jojijac0b and @SanjayVas)


src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ModelRolloutsService.kt line 309 at r3 (raw file):

Previously, Marco-Premier (marcopremier) wrote…

That's because underlying startDate / endDate management in internal APIs did not change. So the service take care of converting dates to prototimes before forwarding requests internally

@SanjayVas - are you okay with this mis-alignment of the internal and public api?

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 2 of 10 files at r1, 2 of 4 files at r2, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Marco-Premier and @stevenwarejones)


src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ModelRolloutsService.kt line 309 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

@SanjayVas - are you okay with this mis-alignment of the internal and public api?

Yes, it's fine. We have something like this in other places as well (off the top of my head, ProtocolConfig is handled differently internally).

One of the reasons we have separate APIs is to allow them to evolve independently, e.g. to avoid data migrations on DBs. Basically we need to weigh whether it's worth changing the internal API whenever we make a public API change.


src/test/kotlin/org/wfanet/measurement/api/v2alpha/tools/MeasurementSystemTest.kt line 1949 at r4 (raw file):

}

// TODO(@MarcoPremier): Move this function to common-jvm.

nit: I think you missed this one


src/test/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ModelRolloutsServiceTest.kt line 1038 at r4 (raw file):

}

// TODO(@MarcoPremier): Move this function to common-jvm.

nit: I think you missed this one.

Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 10 files at r1, 1 of 4 files at r2, 3 of 3 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Marco-Premier)

Copy link
Contributor Author

@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 6 of 10 files at r1, 2 of 4 files at r2, 1 of 1 files at r3, 3 of 3 files at r4.
Reviewable status: 10 of 12 files reviewed, all discussions resolved (waiting on @jojijac0b, @SanjayVas, and @stevenwarejones)

Copy link
Contributor Author

@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 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier)

@Marco-Premier Marco-Premier enabled auto-merge (squash) August 1, 2023 13:46
@Marco-Premier Marco-Premier merged commit 655163e into main Aug 1, 2023
@Marco-Premier Marco-Premier deleted the marcopremier-sync-modelrollout-with-updated-api branch August 1, 2023 14:09
ple13 pushed a commit that referenced this pull request Aug 16, 2024
Replace Timestamps with google.type.Date in ModelRolloutService Fix
broken tests

Internal proto resources remained the same. What changed is the way
public proto resources are converted into internal ones and viceversa.

---------

Co-authored-by: marcopremier <[email protected]>
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