Implement postgres continuation token service

[YuhongWang-Amazon]

This implements the Postgres continuation token service and its dependent Postgres readers/writers.

[wfa-reviewable]

This change is 

[YuhongWang-Amazon]

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

a discussion (no related file):
@SanjayVas @renjiezh please help review this new PR. My previous PR was somehow messed up after a rebase to main branch.

---

[renjiezh]

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

[SanjayVas]

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @renjiezh and @YuhongWang-Amazon)

a discussion (no related file):
nit: PR description should be imperative ("Implement" rather than "Implemented"). See https://github.com/world-federation-of-advertisers/cross-media-measurement/blob/main/docs/dev-standards.md#pull-request-description

---

a discussion (no related file):
Document all public types and members aside from self-explanatory functions. See https://developer.android.com/kotlin/style-guide#usage. (I know that existing code doesn't do a great job at this).

Note that you don't need to document overridden members as those should be documented in the base type.

---

---

src/main/kotlin/org/wfanet/measurement/duchy/deploy/postgres/PostgresContinuationTokensService.kt line 43 at r1 (raw file):

      getContinuationTokenResponse { token = it.continuationToken }
    }
      ?: GetContinuationTokenResponse.getDefaultInstance()

nit: don't need to make this a "one-liner" using ?.let.

Suggestion:

    val result: ContinuationTokenReader.Result = ContinuationTokenReader().getContinuationToken(client.singleUse()) ?: return GetContinuationTokenResponse.getDefaultInstance()
    return getContinuationTokenResponse { token = result.continuationToken }
      

---

src/main/kotlin/org/wfanet/measurement/duchy/deploy/postgres/PostgresContinuationTokensService.kt line 52 at r1 (raw file):

      SetContinuationToken(request.token).execute(client, idGenerator)
    } catch (e: ContinuationTokenInvalidException) {
      throw e.asStatusRuntimeException(Status.FAILED_PRECONDITION.code)

Use the enum value directly rather than grabbing it from a pre-defined Status object.

Suggestion:

Status.Code.FAILED_PRECONDITION

---

src/main/kotlin/org/wfanet/measurement/duchy/deploy/postgres/PostgresContinuationTokensService.kt line 53 at r1 (raw file):

    } catch (e: ContinuationTokenInvalidException) {
      throw e.asStatusRuntimeException(Status.FAILED_PRECONDITION.code)
    } catch (e: InvalidProtocolBufferException) {

Catch this inside of the writer so that the caller only needs to handle specific DuchyInternalExceptions.

Code quote:

 } catch (e: InvalidProtocolBufferException) {

---

src/main/kotlin/org/wfanet/measurement/duchy/deploy/postgres/writers/SetContinutationToken.kt line 26 at r1 (raw file):

import org.wfanet.measurement.system.v1alpha.StreamActiveComputationsContinuationToken

class SetContinuationToken(private val continuationToken: String) : PostgresWriter<Unit>() {

Document class, including which exceptions can be expected on the execute method.

/**
  * [PostgresWriter] for setting continuation tokens.
  *
  * Throws the following exceptions on [execute]:
  * * [ContinuationTokenInvalidException] when the new token is invalid
  */

---

src/main/proto/wfa/measurement/internal/duchy/BUILD.bazel line 10 at r1 (raw file):

package(default_visibility = ["//:__subpackages__"])

IMPORT_PREFIX = "/src/main/proto"

Note that you're redefining IMPORT_PREFIX here after importing it in the load statement above.

---

src/main/proto/wfa/measurement/internal/duchy/BUILD.bazel line 12 at r1 (raw file):

IMPORT_PREFIX = "/src/main/proto"

proto_and_java_proto_library(

Follow the style in the file. In this case, separate targets.

As an aside, I don't think this macro was a good idea. It hides the actual targets from tooling such as IntelliJ.

---

src/main/proto/wfa/measurement/internal/duchy/error_code.proto line 23 at r1 (raw file):

enum ErrorCode {
  UNKNOWN_ERROR = 0;

nit: match API enum style.

Suggestion:

ERROR_CODE_UNSPECIFIED

[YuhongWang-Amazon]

Reviewable status: 7 of 12 files reviewed, 8 unresolved discussions (waiting on @renjiezh and @SanjayVas)

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: PR description should be imperative ("Implement" rather than "Implemented"). See https://github.com/world-federation-of-advertisers/cross-media-measurement/blob/main/docs/dev-standards.md#pull-request-description

I followed that in the PR description. But missed the PR name which was auto generated from the commit message. Updated.

---

---

src/main/kotlin/org/wfanet/measurement/duchy/deploy/postgres/PostgresContinuationTokensService.kt line 52 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Use the enum value directly rather than grabbing it from a pre-defined Status object.

Done.

---

src/main/kotlin/org/wfanet/measurement/duchy/deploy/postgres/PostgresContinuationTokensService.kt line 53 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Catch this inside of the writer so that the caller only needs to handle specific DuchyInternalExceptions.

Done.

---

src/main/kotlin/org/wfanet/measurement/duchy/deploy/postgres/writers/SetContinutationToken.kt line 26 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Document class, including which exceptions can be expected on the execute method.

/**
  * [PostgresWriter] for setting continuation tokens.
  *
  * Throws the following exceptions on [execute]:
  * * [ContinuationTokenInvalidException] when the new token is invalid
  */

Done.

---

src/main/proto/wfa/measurement/internal/duchy/BUILD.bazel line 10 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Note that you're redefining IMPORT_PREFIX here after importing it in the load statement above.

Done.

---

src/main/proto/wfa/measurement/internal/duchy/BUILD.bazel line 12 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Follow the style in the file. In this case, separate targets.

As an aside, I don't think this macro was a good idea. It hides the actual targets from tooling such as IntelliJ.

Done.

[SanjayVas]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @renjiezh and @YuhongWang-Amazon)

---

src/main/kotlin/org/wfanet/measurement/duchy/deploy/postgres/writers/SetContinutationToken.kt line 35 at r2 (raw file):

 *
 * @throws [ContinuationTokenMalformedException] when the new token is malformed
 * @throws [ContinuationTokenInvalidException] when the new token is invalid

Using the @throws tag here is incorrect, as that is implying that the constructor throws these exceptions. Instead, we're just listing what's thrown by the only public method (execute). We need to document those here since we can't add that documentation to the execute method for this subclass (it's intentionally not overridable).

Suggestion:

 * Throws a subclass of [DuchyInternalException] on [execute]:
 * * [ContinuationTokenMalformedException] when the new token is malformed
 * * [ContinuationTokenInvalidException] when the new token is invalid

[SanjayVas]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @renjiezh and @YuhongWang-Amazon)

---

src/main/kotlin/org/wfanet/measurement/duchy/deploy/postgres/writers/SetContinutationToken.kt line 35 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Using the @throws tag here is incorrect, as that is implying that the constructor throws these exceptions. Instead, we're just listing what's thrown by the only public method (execute). We need to document those here since we can't add that documentation to the execute method for this subclass (it's intentionally not overridable).

You can see both good and bad examples in our existing codebase: https://github.com/search?q=repo%3Aworld-federation-of-advertisers%2Fcross-media-measurement+%2Fon+%5C%5Bexecute%5C%5D%2F&type=code

[YuhongWang-Amazon]

Reviewable status: 11 of 12 files reviewed, 3 unresolved discussions (waiting on @renjiezh and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/duchy/deploy/postgres/writers/SetContinutationToken.kt line 35 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

You can see both good and bad examples in our existing codebase: https://github.com/search?q=repo%3Aworld-federation-of-advertisers%2Fcross-media-measurement+%2Fon+%5C%5Bexecute%5C%5D%2F&type=code

To confirm my understanding, when Kdoc generate document for this class, the exceptions decorated by @throws will be captured in a throws section at the class level which could be confusing.
Without @throws, this comment just shows as a text, where readers would read these are actually exceptions throws on [execute] method.

[SanjayVas]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @renjiezh and @YuhongWang-Amazon)

---

src/main/kotlin/org/wfanet/measurement/duchy/deploy/postgres/writers/SetContinutationToken.kt line 35 at r2 (raw file):

Previously, YuhongWang-Amazon wrote…

To confirm my understanding, when Kdoc generate document for this class, the exceptions decorated by @throws will be captured in a throws section at the class level which could be confusing.
Without @throws, this comment just shows as a text, where readers would read these are actually exceptions throws on [execute] method.

Yes.

[YuhongWang-Amazon]

Reviewable status: 11 of 12 files reviewed, 2 unresolved discussions (waiting on @renjiezh and @SanjayVas)

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Document all public types and members aside from self-explanatory functions. See https://developer.android.com/kotlin/style-guide#usage. (I know that existing code doesn't do a great job at this).

Note that you don't need to document overridden members as those should be documented in the base type.

I add the doc for ContinuationTokenReader.getContinuationToken. All other members/types seem pretty self-explanatory

---

[SanjayVas]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @renjiezh and @YuhongWang-Amazon)

a discussion (no related file):

Previously, YuhongWang-Amazon wrote…

I add the doc for ContinuationTokenReader.getContinuationToken. All other members/types seem pretty self-explanatory

Technically the style guide only makes exceptions for self-explanatory functions and properties. Public types should still have KDoc.

Switching this to a non-blocking comment though.

---