Skip to content

Conversation

@palmere-google
Copy link
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

@palmere-google palmere-google requested a review from a team as a code owner July 16, 2021 08:05
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 16, 2021
@Override
public SplitEnumeratorCheckpoint deserialize(int i, byte[] bytes) throws IOException {
return SplitEnumeratorCheckpoint.parseFrom(bytes);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

mind just making one of these for protos? "BackwardsCompatibleProtoSerializer"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent a bit trying to do this. It sounds like the right thing to do, but the results weren't great.

We end up needing two deserializers anyway because SubscriptionPartitionSplit (the other type needing a serializer) isn't a proto, it's just serialized using a proto, so you end up with a serializer that takes a proto serializer and proxies.

Also type erased generics make the implementation awkward because you have to pass ProtoType::parseFrom into the generic proto serializer constructor as a lambda.

I think this is simpler as is

@palmere-google palmere-google merged commit e4649d5 into master Jul 20, 2021
@palmere-google palmere-google deleted the palmere-dev-checkpoint branch July 20, 2021 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants