Skip to content
This repository has been archived by the owner on Sep 25, 2020. It is now read-only.

Allow multiple checksums #89

Merged
merged 1 commit into from
Dec 7, 2016
Merged

Conversation

mennopruijssers
Copy link
Contributor

uber/ringpop-go#192 adds support for multiple checksums. This PR makes the required changes to the protocol schema.

@@ -115,6 +115,14 @@
"checksum": {
"type": "number"
},
"checksums": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since ringpop-node doesn't support it yet, checksums is not added to the list with required properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

"type": "object",
"patternProperties": {
"^.*$": {
"type": "number"
Copy link
Contributor

Choose a reason for hiding this comment

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

A checksum is always a number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now yes (and I don't see any reason to change it in the future)

Copy link
Contributor

Choose a reason for hiding this comment

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

The result of a farmhash is always a number. That's the bit I was missing (I thought it was alphanumeric). Cool, thanks!

Copy link
Contributor

@dansimau dansimau left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mennopruijssers mennopruijssers merged commit 354dd02 into master Dec 7, 2016
@mennopruijssers mennopruijssers deleted the menno/allow-multi-checksum branch December 7, 2016 12:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants