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

Refactor logs operation #1640

Open
4 tasks
pav-kv opened this issue May 28, 2019 · 4 comments
Open
4 tasks

Refactor logs operation #1640

pav-kv opened this issue May 28, 2019 · 4 comments
Labels

Comments

@pav-kv
Copy link
Contributor

pav-kv commented May 28, 2019

LogOperation and its co-types are poorly designed:

  • They are meant to be stateless, but in reality there are caches in various places like SequencerManager (see the list below). We should rather make an explicit per-tree cache that can be extended (e.g. with compact ranges cached between sequencing runs as in sequencer: Cache compact.Tree between sequencing runs #1598).
  • Interfaces and method signatures are redundant. For example, log.NewSequencer takes a Signer created from a Tree, but then the same Tree is passed into sequencer.IntegrateBatch when in fact there is only one Tree that can be accepted.
  • log_operation_manager.go is meant to be agnostic of sequencing, but it contains a bunch of sequencing-related metrics. It is likely that LogOperation abstraction is YAGNI.
  • TODO: Keep listing changes.

The list of things we cache:

  • Log names (in OperationManager).
  • Signers (in SequencerManager).
  • Masterships (in OperationManager).
  • Compact ranges (not yet).
@pav-kv
Copy link
Contributor Author

pav-kv commented May 28, 2019

The current design has 3 types (OperationManager, SequencerManager and Sequencer), which all have their own caches for individual tree IDs.

I propose to shift "caching" one level up the stack, so that OperationManager merely does couple of things: tracks log IDs and the corresponding masterships, and for each active log ID has an Operation object. Operation object encapsulates all "cached" items that were previously scattered across different types (log names, signers, compact ranges, etc), and contains the code for handling only one log (effectively some blend of former SequencerManager and Sequencer, but with fixed Tree/treeID).

@pav-kv
Copy link
Contributor Author

pav-kv commented May 28, 2019

@Martin2112 @AlCutter WDYT?

@Martin2112
Copy link
Contributor

Yes the structure could be improved. If you're going to cache tree related things then it must be done in a way that's completely safe. You might want to make that a separate work item.

@pav-kv
Copy link
Contributor Author

pav-kv commented May 28, 2019

Yep. I will probably start from just restructuring the code with equivalent safety guarantees. For example, consider SequencerManager and its Signers cache: it is never cleared, and its items are never updated. Same with OperationManager and log names.

Once it's restructured, we could improve guarantees. E.g. if an Operation fails, we can delete it altogether, so on the next run it will be re-created with fresh signer, log name, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants