Create Population Requisition Fulfiller Daemon.

[jojijac0b]

Creates daemon for Population Requisition Fulfiller

[wfa-reviewable]

This change is 

[SanjayVas]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jojijac0b, @kungfucraig, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 143 at r2 (raw file):

  @CommandLine.Option(
    names = ["--event-message-descriptor-set"],
    description = ["Serialized DescriptorSet for the event message and its dependencies."],

Suggestion:

FileDescriptorSet

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 143 at r2 (raw file):

  @CommandLine.Option(
    names = ["--event-message-descriptor-set"],
    description = ["Serialized DescriptorSet for the event message and its dependencies."],

Document this this can be specified multiple times.

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 150 at r2 (raw file):

  @Option(
    names = ["--pdp-data"],
    description = ["The Population DataProvider's public API resource name."],

Description doesn't match type

Code quote:

"The Population DataProvider's public API resource name."

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 156 at r2 (raw file):

  @Option(
    names = ["--measurement-consumer-name"],

The other flags for resource names end in resource-name. Have them all be consistent.

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 172 at r2 (raw file):

    required = true,
  )
  private lateinit var populationInfoFileMap: Map<String, List<File>>

Do not do this. We never want repeated elements in a flag value, and instead want flags to be specified multiple times. This means having separate --population-spec flag for the specs and another for the descriptors.

Note that I'm also confused on what FileDescriptorSet this is supposed to be, since there's already a flag for the FileDescriptorSet for the event message which will therefore contain all event template descriptors.

Code quote:

List<File>

[stevenwarejones]

If you wanted to add a test @jojijac0b , it would be something like this

val args = arrayOf(<flags>)
val daemon = PopulationRequisitionFulfillerDaemon()
val cmd = picocli.CommandLine(daemon)
val exitCode = cmd.execute(*args)
assertThat(exitCode).isEqualTo(0)
<assert other things>

I'm not sure what you'd actually test though since you need other components in order to properly test. That's what the correctness test is for.

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jojijac0b, @kungfucraig, and @option)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 153 at r2 (raw file):

    required = true,
  )
  private lateinit var pdpDataFile: File

what is this used for?

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 160 at r2 (raw file):

    required = true,
  )
  private lateinit var measurementConsumerName: String

can someone remind me why the measurement consumer name is a flag that is passed in here? That doesn't make any sense to me. I must be missing something. It feels like simulator code that should not be part of the abstract RequisitionFulfiller -

cross-media-measurement/src/main/kotlin/org/wfanet/measurement/dataprovider/RequisitionFulfiller.kt

Line 78 in d711fd9

protected val measurementConsumerName: String,

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 172 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Do not do this. We never want repeated elements in a flag value, and instead want flags to be specified multiple times. This means having separate --population-spec flag for the specs and another for the descriptors.

Note that I'm also confused on what FileDescriptorSet this is supposed to be, since there's already a flag for the FileDescriptorSet for the event message which will therefore contain all event template descriptors.

This is for populating Map<PopulationKey, PopulationInfo>, right? PopulationInfo contains both PopulationSpec and a Descriptor. I would think we'd want repeating argument groups to capture this right? https://picocli.info/#_repeating_composite_argument_groups

Something like

    @ArgGroup(exclusive = false, multiplicity = "1..*")
    List<PopulationKeyAndInfo> populationKeyAndInfo

    static class PopulationKeyAndInfo {
        @Option(names = "--population-key", required = true)
        String populationKey

        @ArgGroup(exclusive = false, multiplicity=1)
        PopulationInfo populationInfo
    }

    static class PopulationInfo {
        @Option(names = "--population-spec", required = true)
        private lateinit var populationSpecFile File;
        @Option(names = "--event-message-descriptor", required = true)
        private lateinit var eventMessageDescriptorFile File;
    }

[stevenwarejones]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jojijac0b, @kungfucraig, and @option)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 160 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

can someone remind me why the measurement consumer name is a flag that is passed in here? That doesn't make any sense to me. I must be missing something. It feels like simulator code that should not be part of the abstract RequisitionFulfiller -

cross-media-measurement/src/main/kotlin/org/wfanet/measurement/dataprovider/RequisitionFulfiller.kt

Line 78 in d711fd9

protected val measurementConsumerName: String,

@SanjayVas

[SanjayVas]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jojijac0b, @kungfucraig, @option, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 160 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

@SanjayVas

The event requisition fulfiller (EDP simulator) explicitly only works for a single MC. There's indeed no need to have this be the case for the population requisition fulfiller. Furthermore, the value isn't even used by the base RequisitionFulfiller and is instead used in its subclasses.

[kungfucraig]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @jojijac0b, @option, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 241 at r2 (raw file):

  }

  private fun buildTypeRegistry(): TypeRegistry {

I'd recommend putting these in another file and writing unit tests for them.

[stevenwarejones]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @jojijac0b and @option)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 160 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The event requisition fulfiller (EDP simulator) explicitly only works for a single MC. There's indeed no need to have this be the case for the population requisition fulfiller. Furthermore, the value isn't even used by the base RequisitionFulfiller and is instead used in its subclasses.

agreed. please factor this out of the abstract class (I think we all missed that when previously reviewed) and only include it in the edp simulator @jojijac0b

[jojijac0b]

Reviewable status: 1 of 6 files reviewed, 8 unresolved discussions (waiting on @kungfucraig, @option, @SanjayVas, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 143 at r2 (raw file):

  @CommandLine.Option(
    names = ["--event-message-descriptor-set"],
    description = ["Serialized DescriptorSet for the event message and its dependencies."],

Done.

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 150 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Description doesn't match type

Removed

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 153 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

what is this used for?

Removed as this information is gathered from the "flags" flag

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 160 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The event requisition fulfiller (EDP simulator) explicitly only works for a single MC. There's indeed no need to have this be the case for the population requisition fulfiller. Furthermore, the value isn't even used by the base RequisitionFulfiller and is instead used in its subclasses.

Removed MeasurementConsumerName parameter from RequisitionFulfiller superclass. It is now only used in the EDPSimulator sub-class

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 172 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

This is for populating Map<PopulationKey, PopulationInfo>, right? PopulationInfo contains both PopulationSpec and a Descriptor. I would think we'd want repeating argument groups to capture this right? https://picocli.info/#_repeating_composite_argument_groups

Something like

    @ArgGroup(exclusive = false, multiplicity = "1..*")
    List<PopulationKeyAndInfo> populationKeyAndInfo

    static class PopulationKeyAndInfo {
        @Option(names = "--population-key", required = true)
        String populationKey

        @ArgGroup(exclusive = false, multiplicity=1)
        PopulationInfo populationInfo
    }

    static class PopulationInfo {
        @Option(names = "--population-spec", required = true)
        private lateinit var populationSpecFile File;
        @Option(names = "--event-message-descriptor", required = true)
        private lateinit var eventMessageDescriptorFile File;
    }

Uses @Arggroup

[SanjayVas]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b, @option, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 172 at r2 (raw file):

Previously, jojijac0b wrote…

Uses @Arggroup

What I'm getting at is that this overlaps the top-level --event-message-descriptor-set option. I think what you actually want is having the FileDescriptorSet files passed to that option, and have a string --event-message-type-url for each PopulationSpec . You can use the type URL to look up the Descriptor from the loaded FileDescriptorSets.

[stevenwarejones]

Reviewed 4 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jojijac0b and @option)

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 199 at r3 (raw file):

  }

  val measurementConsumerKey =

this can be private right?

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 158 at r3 (raw file):

    @CommandLine.ArgGroup(exclusive = false, multiplicity = "1")
    lateinit var populationInfo: PopulationInfo

is it supported to pass in data classes this way?

[jojijac0b]

Reviewable status: 5 of 6 files reviewed, 7 unresolved discussions (waiting on @option, @SanjayVas, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 172 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

What I'm getting at is that this overlaps the top-level --event-message-descriptor-set option. I think what you actually want is having the FileDescriptorSet files passed to that option, and have a string --event-message-type-url for each PopulationSpec . You can use the type URL to look up the Descriptor from the loaded FileDescriptorSets.

Done. Changed the eventMessageDescriptorFile to eventMessageTypeUrl(of type string). The buildPopulationInfoMap() method will use the type registry built using the eventMessageDescriptorSetFiles input and this eventMessageTypeUrl to get the descriptor for that PopulationInfo object.

[jojijac0b]

Reviewable status: 3 of 6 files reviewed, 7 unresolved discussions (waiting on @option, @SanjayVas, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 143 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Document this this can be specified multiple times.

Done.

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 156 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The other flags for resource names end in resource-name. Have them all be consistent.

Done.

[jojijac0b]

Reviewable status: 3 of 6 files reviewed, 7 unresolved discussions (waiting on @option, @SanjayVas, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 158 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

is it supported to pass in data classes this way?

This is referencing the PopulationInfo class below. I believe it is possible. I see a similar design here:

cross-media-measurement/src/main/kotlin/org/wfanet/measurement/reporting/service/api/v1alpha/tools/Reporting.kt

Line 225 in 63d2dc5

private lateinit var eventGroups: List<EventGroupInput>

[stevenwarejones]

Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: 5 of 6 files reviewed, 6 unresolved discussions (waiting on @kungfucraig, @option, and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 241 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

I'd recommend putting these in another file and writing unit tests for them.

we've done this plenty of times without writing unit tests
https://github.com/search?q=repo%3Aworld-federation-of-advertisers%2Fcross-media-measurement%20buildtyperegistry&type=code

[SanjayVas]

Reviewed 1 of 1 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jojijac0b, @kungfucraig, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 156 at r2 (raw file):

Previously, jojijac0b wrote…

Done.

The intent was for this to be --population-resource-name instead of --population-key.

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 158 at r3 (raw file):

is it supported to pass in data classes this way?

This usage is supported because it isn't a data class, but rather another type containing flags that's mixed in.

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 141 at r6 (raw file):

    private set

  @Option(

Why are some flags defined here and some in the Flags class above? You don't need a separate Flags class if it's just used in one place. You move all the properties from PopulationRequisitionFulfillerFlags here, switching them to be private rather than being public with private set.

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 149 at r6 (raw file):

  @CommandLine.ArgGroup(exclusive = false, multiplicity = "1..*")
  lateinit var populationKeyAndInfo: List<PopulationKeyAndInfo>

private lateinit var and drop the private set

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 152 at r6 (raw file):

    private set

  class PopulationKeyAndInfo {

nit: I believe this class can be private and still be used by

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 200 at r6 (raw file):

      )

    val channelTarget = flags.target

nit: specify types when not obvious, i.e. when not calling a constructor or well-known factory function

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 203 at r6 (raw file):

    val channelCertHost = flags.certHost

    val certificatesChannel = buildMutualTlsChannel(channelTarget, clientCerts, channelCertHost)

Share a single channel for all stubs pointing to the same target. See https://grpc.io/docs/guides/performance/#general

[jojijac0b]

Reviewable status: 5 of 6 files reviewed, 7 unresolved discussions (waiting on @kungfucraig, @SanjayVas, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 156 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The intent was for this to be --population-resource-name instead of --population-key.

Done.

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 141 at r6 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Why are some flags defined here and some in the Flags class above? You don't need a separate Flags class if it's just used in one place. You move all the properties from PopulationRequisitionFulfillerFlags here, switching them to be private rather than being public with private set.

Done.

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 149 at r6 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

private lateinit var and drop the private set

Done.

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 203 at r6 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Share a single channel for all stubs pointing to the same target. See https://grpc.io/docs/guides/performance/#general

Done.

[SanjayVas]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b, @kungfucraig, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 49 at r9 (raw file):

class PopulationRequisitionFulfillerDaemon : Runnable {
  @Command(

The @Command annotation goes on the class.

---

src/main/kotlin/org/wfanet/measurement/populationdataprovider/PopulationRequisitionFulfillerDaemon.kt line 139 at r9 (raw file):

  private class PopulationKeyAndInfo {
    @Option(names = ["--population-resource-name"], required = true)
    lateinit var populationKey: String

Rename this too

Suggestion:

populationResourceName