-
Notifications
You must be signed in to change notification settings - Fork 366
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
[INDY-1327] change key-value in ReqIdrToTxn #712
[INDY-1327] change key-value in ReqIdrToTxn #712
Conversation
Changes - now ReqIdrToTxn store data in map digest -> ledge_id<delimiter>seq_no Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
…nto task-1327-req-idr-to-txn-change
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>
[INDY-1327] add protocol version getting
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Changes: - fix test client_authn.py - fix test test_req_to_txn.py Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
test this please |
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
test this please |
2 similar comments
test this please |
test this please |
test this please |
1 similar comment
test this please |
Test this please |
…nto task-1327-req-idr-to-txn-change
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
plenum/persistence/req_id_to_txn.py
Outdated
@@ -11,30 +11,36 @@ class ReqIdrToTxn: | |||
""" | |||
|
|||
def __init__(self, keyValueStorage: KeyValueStorage): | |||
self.delimiter = "~" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be a static constant on class level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
plenum/persistence/req_id_to_txn.py
Outdated
parse_data = val.split(self.delimiter) | ||
return str(parse_data[0]), str(parse_data[1]) | ||
|
||
def _create_value(self, ledge_id, seq_no): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: ledge_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
plenum/persistence/req_id_to_txn.py
Outdated
|
||
def _parse_value(self, val: string): | ||
parse_data = val.split(self.delimiter) | ||
return str(parse_data[0]), str(parse_data[1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we expected seqNo as int previously and not it's str. Does it break anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to return seqNo as int, but I don't sure that it's a good idea. In fact, we store in a seqNoDB string. And in the origin version of code we convert it to int after get() just in case.
plenum/persistence/req_id_to_txn.py
Outdated
@@ -11,30 +11,36 @@ class ReqIdrToTxn: | |||
""" | |||
|
|||
def __init__(self, keyValueStorage: KeyValueStorage): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have unit tests for ReqIdrToTxn
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have only integration test in test_already_processed_requests(). Should to add unit tests too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -120,6 +120,10 @@ def get_req_id(txn): | |||
return txn[TXN_PAYLOAD][TXN_PAYLOAD_METADATA].get(TXN_PAYLOAD_METADATA_REQ_ID, None) | |||
|
|||
|
|||
def get_digest(txn): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test for get_digest
to test_txn_general_access_utils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
plenum/server/pool_manager.py
Outdated
@@ -196,7 +196,7 @@ def executePoolTxnBatch(self, ppTime, reqs, stateRoot, txnRoot) -> List: | |||
:param reqs: request | |||
""" | |||
committedTxns = self.reqHandler.commit(len(reqs), stateRoot, txnRoot, ppTime) | |||
self.node.updateSeqNoMap(committedTxns) | |||
self.node.updateSeqNoMap(committedTxns, DOMAIN_LEDGER_ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean POOL_LEDGER_ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you. I'm not sure, because this information is not correct to leave as a constant, but I didn't find another way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
req.get(f.SIGS.nm, None), | ||
req.get(f.PROTOCOL_VERSION.nm, None) | ||
).digest | ||
sign = req.get("signature") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please replace "signature" with f.SIG.nm as you use before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Changes: - add test test_get_digest() - refactoring req_and_expected() - refactoring ReqIdrToTxn Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
elif isinstance(req, str): | ||
req_data = json.loads(req) | ||
elif isinstance(req, Request): | ||
req = Request( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use named arguments here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -246,7 +259,8 @@ def do_req_to_txn(req_data, req_op): | |||
|
|||
append_payload_metadata(result, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test to test_txn_init_utils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
test this please |
2 similar comments
test this please |
test this please |
Changes: