[WIP]INDY-1874 General Tracker #1062
Conversation
Signed-off-by: Cam Parra <caeparra@gmail.com>
Signed-off-by: Cam Parra <caeparra@gmail.com>
Signed-off-by: Cam Parra <caeparra@gmail.com>
Signed-off-by: Cam Parra <caeparra@gmail.com>
Signed-off-by: Cam Parra <caeparra@gmail.com>
Signed-off-by: Cam Parra <caeparra@gmail.com>
plenum/common/tracker.py
Outdated
|
|
||
| self.un_committed.append(un_committed_state) | ||
|
|
||
| def reject_batch(self): |
There was a problem hiding this comment.
Reject batch should return the latest uncommitted values, so that they can be used in a real revert of a state and ledger.
plenum/common/tracker.py
Outdated
| @@ -0,0 +1,33 @@ | |||
| from stp_core.common.log import Logger | |||
|
|
|||
| logger = Logger() | |||
There was a problem hiding this comment.
Please use logger = getlogger() if you need a logger
plenum/common/tracker.py
Outdated
| if state_root != "": | ||
| un_committed_state += (state_root,) | ||
| else: | ||
| raise logger.error("Empty state root given") |
There was a problem hiding this comment.
You are raising a logger output (which is not an exception).
You can use raise LogicError or raise PlenumValueError instead.
plenum/common/tracker.py
Outdated
| raise logger.error("Incorrect size of ledger given") | ||
|
|
||
| self.un_committed.append(un_committed_state) | ||
|
|
There was a problem hiding this comment.
We should also have an API call commit_batch() which should remove the first value from the un_committed.
So, taking into account that we deal with the following 3 operation: pop left, pop right, append right, I think it's worth considering deque as a structure for un_committed.
plenum/common/tracker.py
Outdated
| logger = Logger() | ||
|
|
||
|
|
||
| class Tracker: |
There was a problem hiding this comment.
What about LedgerUncommitedTracker name for this class?
plenum/common/tracker.py
Outdated
| def __init__(self): | ||
| self.un_committed = [] | ||
|
|
||
| def track(self, state_root, ledger_size): |
There was a problem hiding this comment.
Maybe call it apply_batch?
…rrors Signed-off-by: Cam Parra <caeparra@gmail.com>
Signed-off-by: Cam Parra <caeparra@gmail.com>
Signed-off-by: Cam Parra <caeparra@gmail.com>
|
@ashcherbakov I have made the changes you requested. Please review. I think that this needs integration tests |
Signed-off-by: Cam Parra <caeparra@gmail.com>
|
|
||
| un_committed_state = () | ||
|
|
||
| if state_root != "": |
There was a problem hiding this comment.
Is it expected that we use "" and not, for example, None to detect that no state root is passed?
If we want a real validation of the state root here, then we may use MerkleRootField.
But actually I think that for such a simple class this validation may be a bit too complex and time consuming.
I would say that it's enough to check for None only here, so that
# validation
if state_root is None:
raise PlenumValueError
if ledger_size is None or ledger_size <= 0:
raise PlenumValueError
self.un_committed.append(state_root, ledger_size ))
There was a problem hiding this comment.
I agree with this. I will make these changes
| make_tracker.reject_batch() | ||
|
|
||
|
|
||
| def test_build_uncommitted_and_reject_batch(make_tracker): |
There was a problem hiding this comment.
Can you please add more tests for each API method?
Also it makes sense to test that calling apply_batch and/or revert_batch works as expected.
Signed-off-by: Cam Parra <caeparra@gmail.com>
Signed-off-by: Cam Parra <caeparra@gmail.com>
Signed-off-by: Cam Parra <caeparra@gmail.com>
Signed-off-by: Cam Parra <caeparra@gmail.com>
| test_tuple = ("test_root", 1000) | ||
| make_tracker.apply_batch(test_tuple[0], test_tuple[1]) | ||
| [make_tracker.apply_batch(state_root, i + 1) for i in range(1, 9)] | ||
| assert make_tracker.commit_batch() == test_tuple |
There was a problem hiding this comment.
It would be great to have tests calling commit_batch multiple times in a row. The same for reject_batch.
Signed-off-by: Cam Parra caeparra@gmail.com