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.

Sign up for GitHub

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

Jump to bottom

Move TFDV stats fields on FeatureSpec proto to higher-numbered fields #667

Closed
ches opened this issue May 1, 2020 · 0 comments · Fixed by #669
Closed

Move TFDV stats fields on FeatureSpec proto to higher-numbered fields #667

ches opened this issue May 1, 2020 · 0 comments · Fixed by #669
Assignees
@ches
Labels
kind/techdebt
Milestone
v0.5.0

Comments

@ches
Copy link
Member

ches commented May 1, 2020

As discussed at #655 (comment) and according to protobuf spec guidance:

…field numbers in the range 1 through 15 take one byte to encode, including the field number and the field's type (you can find out more about this in Protocol Buffer Encoding). Field numbers in the range 16 through 2047 take two bytes. So you should reserve the numbers 1 through 15 for very frequently occurring message elements. Remember to leave some room for frequently occurring elements that might be added in the future.

Although FeatureSpec is registry (meta)data that is not used in high-throughput RPCs or Kafka messages currently, it doesn't seem like a bad idea to move the TFDV stats and reserve low-number fields while we can. The cost is low.
@ches ches added this to the v0.5.0 milestone May 1, 2020
@ches ches self-assigned this May 1, 2020
ches added a commit to agoda-com/feast that referenced this issue May 1, 2020
@ches
Move TFDV stats to higher-numbered protobuf fields
75bd8d6 
Within the Feast v0.5 release cycle these fields have not been in a
release yet, so it's safe to renumber them.

The motivation is reserving the lower-numbered fields for optimal
encoding, referenced in the patch.

Closes feast-dev#667
@ches ches mentioned this issue May 1, 2020
Merged
feast-ci-bot pushed a commit that referenced this issue May 1, 2020
@ches
Move TFDV stats to higher-numbered protobuf fields (#669)
e014733 
Within the Feast v0.5 release cycle these fields have not been in a
release yet, so it's safe to renumber them.

The motivation is reserving the lower-numbered fields for optimal
encoding, referenced in the patch.

Closes #667
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ches ches

Labels
kind/techdebt
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging a pull request may close this issue.

Move TFDV stats to higher-numbered protobuf fields agoda-com/feast
1 participant
@ches