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

Consolidate View and SeqNo typedefs #2282

Closed
eddyashton opened this issue Mar 9, 2021 · 2 comments · Fixed by #2367
Closed

Consolidate View and SeqNo typedefs #2282

eddyashton opened this issue Mar 9, 2021 · 2 comments · Fixed by #2367
Assignees

Comments

@eddyashton
Copy link
Member

We currently have a lot of different typedefs for View and SeqNo (under kv::, kv::Consensus, etc), as well as some older terminology referring to Version, Index, and Term. As part of #697, the former should be consolidated into a single alias (ccf::View, ccf::SeqNo, from ccf/tx_id.h), and the latter should only be used inside the KV/Raft and not exposed beyond them.

@achamayou
Copy link
Member

We should kill off all remaining usage of Term now that TxView is gone.

@eddyashton eddyashton self-assigned this Mar 10, 2021
@eddyashton
Copy link
Member Author

I'm making progress on this, with a few caveats:

  • We were benefitting from a lot of direct conversions from kv::Version (signed so it can handle deletions) and SeqNo (unsigned). I think a lot of the uses of Version should actually be unsigned - wherever we use it to refer to an entire Tx for instance, it must be non-deleted. So I'm trending towards making these all unsigned, but maintaining a signed internal representation for deletions in the champ. This would be easier if we did Permanently delete items in the KV champ maps #1992 first, but I think we need to do it this way round to smooth off the API ASAP.
  • This already includes a lot of hairy guesswork of "is this KV or everything-else", so I'm avoiding removing Term and Version terminology for now. Once they're all unsigned and the distinction is drawn, it should be easier to rename Term to View in isolation.

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 a pull request may close this issue.

2 participants