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

Introduce the concept of known EventGroup metadata types. #1512

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

SanjayVas
Copy link
Member

@SanjayVas SanjayVas commented Feb 29, 2024

Known EventGroup metadata types are types expected to be known by all callers. These need not be included in the EventGroupMetadataDescriptor FileDescriptorSet, and are instead loaded separately.

This also enforces uniqueness for EventGroupMetadataDescriptor message types. Note that this only affects the creation of new resources. Existing resources may exist that already violate this constraint.

Includes a workaround for bazelbuild/rules_proto#203.

Closes #1511

@wfa-reviewable
Copy link

This change is Reviewable

@SanjayVas SanjayVas force-pushed the sanjayvas-metadata-types branch from 5a84e21 to 2434820 Compare February 29, 2024 22:36
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 9 of 10 files at r1, all commit messages.
Reviewable status: 9 of 10 files reviewed, all discussions resolved

@SanjayVas SanjayVas force-pushed the sanjayvas-metadata-types branch from 2434820 to 56e626e Compare March 6, 2024 01:18
@SanjayVas SanjayVas changed the title Enforce uniqueness of EventGroupMetadataDescriptor message types. Introduce the concept of known EventGroup metadata types. Mar 6, 2024
@SanjayVas
Copy link
Member Author

SanjayVas commented Mar 6, 2024

This was significantly reworked to account for the fact that some metadata message types should be shared. One example is SyntheticEventGroupSpec. PTAL.

Depends on world-federation-of-advertisers/common-jvm#235

CC @robinsons

@SanjayVas SanjayVas force-pushed the sanjayvas-metadata-types branch from 56e626e to ce8facb Compare March 6, 2024 22:27
@SanjayVas SanjayVas force-pushed the sanjayvas-metadata-types branch 3 times, most recently from d24e8e7 to 2b26256 Compare March 7, 2024 21:08
@SanjayVas SanjayVas changed the base branch from main to sanjayvas-panelmatch-wait March 7, 2024 21:09
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.

What's the use case for sharing metadata messages?

Reviewed 19 of 59 files at r2, 1 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 29 of 68 files reviewed, all discussions resolved

@SanjayVas SanjayVas force-pushed the sanjayvas-panelmatch-wait branch from 22fa87d to 33ff31e Compare March 7, 2024 23:25
@SanjayVas SanjayVas force-pushed the sanjayvas-metadata-types branch from 2b26256 to 56f32f9 Compare March 7, 2024 23:25
Copy link
Member Author

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

  1. We use shared metadata message types for the simulator test event groups
  2. A given market may want all EventGroups across all EDPs to have a field with some common message type

Reviewable status: 29 of 68 files reviewed, all discussions resolved (waiting on @stevenwarejones)

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.

  1. Will this be the case? EDPs probably use different taxonomies and will need different event group metadata.

Reviewable status: 29 of 68 files reviewed, all discussions resolved

Copy link
Member Author

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

A field with a common message type is not the whole message type. Basically the metadata can have a common part and an EDP-specific part. IIUC, having some commonality is a desire that has already been expressed by Origin.

Reviewable status: 29 of 68 files reviewed, all discussions resolved (waiting on @stevenwarejones)

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 38 of 59 files at r2, 1 of 3 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

@SanjayVas SanjayVas force-pushed the sanjayvas-metadata-types branch from 56f32f9 to 8f0e67a Compare March 8, 2024 23:21
@SanjayVas SanjayVas force-pushed the sanjayvas-panelmatch-wait branch from 33ff31e to f8918a7 Compare March 8, 2024 23:21
@SanjayVas SanjayVas force-pushed the sanjayvas-metadata-types branch from 8f0e67a to d60aaed Compare March 8, 2024 23:26
@SanjayVas SanjayVas force-pushed the sanjayvas-panelmatch-wait branch 2 times, most recently from a813002 to 69baa4c Compare March 9, 2024 01:30
@SanjayVas SanjayVas force-pushed the sanjayvas-metadata-types branch 2 times, most recently from f85b958 to 49d33b3 Compare March 9, 2024 01:58
@SanjayVas SanjayVas force-pushed the sanjayvas-panelmatch-wait branch from 69baa4c to b426714 Compare March 9, 2024 01:58
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 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

@SanjayVas SanjayVas force-pushed the sanjayvas-metadata-types branch from 49d33b3 to 49cdf6c Compare March 11, 2024 16:52
@SanjayVas SanjayVas force-pushed the sanjayvas-panelmatch-wait branch from b426714 to cf321bf Compare March 11, 2024 16:52
Base automatically changed from sanjayvas-panelmatch-wait to main March 11, 2024 17:34
@SanjayVas SanjayVas force-pushed the sanjayvas-metadata-types branch from 49cdf6c to 4809980 Compare March 11, 2024 17:41
Known types are types expected to be known by all callers. These need not be included in the EventGroupMetadataDescriptor FileDescriptorSet, and are instead loaded separately.

This also enforces uniqueness for EventGroupMetadataDescriptor message types. Note that this only affects the creation of new resources. Existing resources may exist that already violate this constraint.
@SanjayVas SanjayVas force-pushed the sanjayvas-metadata-types branch from 4809980 to 83f6ac4 Compare March 11, 2024 18:50
Copy link
Member Author

@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 2 files at r6.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

@SanjayVas SanjayVas enabled auto-merge (squash) March 11, 2024 19:02
@SanjayVas SanjayVas merged commit bfdf135 into main Mar 11, 2024
4 checks passed
@SanjayVas SanjayVas deleted the sanjayvas-metadata-types branch March 11, 2024 19:31
SanjayVas added a commit that referenced this pull request Apr 2, 2024
ple13 pushed a commit that referenced this pull request Aug 16, 2024
Known EventGroup metadata types are types expected to be known by all callers. These need not be included in the EventGroupMetadataDescriptor FileDescriptorSet, and are instead loaded separately.

This also enforces uniqueness for EventGroupMetadataDescriptor message types. Note that this only affects the creation of new resources. Existing resources may exist that already violate this constraint.

Includes a workaround for bazelbuild/rules_proto#203.

Closes #1511
ple13 pushed a commit that referenced this pull request Aug 16, 2024
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.

Enforce message type uniqueness for EventGroupMetadataDescriptors
3 participants