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

fix(pubsublite): rename publish.Metadata to pscompat.MessageMetadata #3672

Merged
merged 7 commits into from Feb 11, 2021

Conversation

@tmdiep
Copy link
Contributor

@tmdiep tmdiep commented Feb 4, 2021

Since publish.Metadata now also applies to message ids, it was renamed to MessageMetadata. It is duplicated in the internal/wire and pscompat packages.

Follow up for #3662.

See also the Java equivalent googleapis/java-pubsublite#482.

@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Feb 4, 2021

@tbpg @codyoss @hongalex
FYI this is a minor API change - rename of the publish subpackage to types. It contains utils for parsing some Pub/Sub Lite specific info that we had to encode into string fields of pubsub types.

@tbpg
Copy link
Collaborator

@tbpg tbpg commented Feb 4, 2021

A types package is generally discouraged in Go. Here is a great explanation from rakyll: https://rakyll.org/style-packages/#organize-by-responsibility.

If we need this in a separate package (I think we do?), consider the name message? Then we have message.Metadata and message.ParseMetadata.

@tmdiep tmdiep force-pushed the message_metadata branch from c4ee1d1 to bc9a05b Feb 5, 2021
@tmdiep tmdiep force-pushed the message_metadata branch from bc9a05b to a93eeca Feb 5, 2021
@tmdiep tmdiep changed the title fix(pubsublite): rename publish.Metadata to types.MessageMetadata fix(pubsublite): rename publish.Metadata to message.Metadata Feb 5, 2021
@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Feb 5, 2021

@tbpg Thanks for your offline suggestion about having the Parse function just return the fields, and removing the subpackage entirely. Discussed with other folks and we generally prefer parity and consistency with the other languages. This feature was requested by a user for the Java lib and we expect a small, but non-trivial number of users to need it.

So I renamed the package and struct to message.Metadata.

There is another option - move the struct to internal/wire, add a type alias and Parse function to pscompat. But feels a little clunky and since it's a seldom used util, I prefer to keep it out of the main packages.

@tbpg
Copy link
Collaborator

@tbpg tbpg commented Feb 5, 2021

Thanks for your offline suggestion about having the Parse function just return the fields, and removing the subpackage entirely. Discussed with other folks and we generally prefer parity and consistency with the other languages. This feature was requested by a user for the Java lib and we expect a small, but non-trivial number of users to need it.

I don't understand how not having the type or extra package makes the Go library not have parity with the other languages. What is the benefit of a type over returning the fields directly? The GoDoc will document exactly what is returned. If it's in another package, it will be harder to find, there is another package for users to have to import, etc. Ideally, the function would show up right next to the thing users will need to use it with.

@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Feb 8, 2021

@tbpg @manuelmenzella-google

I don't understand how not having the type or extra package makes the Go library not have parity with the other languages. What is the benefit of a type over returning the fields directly?

We wanted a struct mostly for consistency with the other languages. Extensibility if we encode more fields (e.g. we may need to add an epoch to the partition number for repartitioning).

If it's in another package, it will be harder to find, there is another package for users to have to import, etc. Ideally, the function would show up right next to the thing users will need to use it with.

Good point. Maybe pscompat.MessageMetadata then? It will have to be a type alias for the actual struct in internal/wire, unfortunately, due to import cyclic dependencies.

The pubsublite package kind of works too, since it is Lite-specific, but it's not used with anything in that package.

@tmdiep tmdiep changed the title fix(pubsublite): rename publish.Metadata to message.Metadata fix(pubsublite): rename publish.Metadata to pscompat.MessageMetadata Feb 9, 2021
@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Feb 9, 2021

@tbpg @codyoss

There is now a pscompat.MessageMetadata struct (which is actually an alias due to cyclic dependencies) and pscompat.ParseMessageMetadata function.

LMK if you have any concerns from an API perspective. Thanks!

tbpg
tbpg approved these changes Feb 11, 2021
Copy link
Collaborator

@tbpg tbpg left a comment

Thanks! Looks good to me.

I left one question about duplicating vs. aliasing. Can see it both ways.

pubsublite/pscompat/message.go Outdated Show resolved Hide resolved
@tmdiep tmdiep merged commit 6a8d4c5 into googleapis:master Feb 11, 2021
3 checks passed
@tmdiep tmdiep deleted the message_metadata branch Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants