-
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-1945: Implement NodeLeecherService #1120
Conversation
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
self._state = self.State.SyncingAudit | ||
self._leechers[AUDIT_LEDGER_ID].start(request_ledger_statuses) | ||
|
||
def _on_ledger_leecher_stop(self, msg: LedgerCatchupComplete): |
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.
Is it used somewhere?
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 should be used, I missed adding message routing
logger.warning("{} got unexpected catchup complete {} during idle state".format(self, msg)) | ||
|
||
def _on_audit_synced(self, msg): | ||
if msg.ledger_id != AUDIT_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.
As of now, _current_ledger
is used only when syncing other ledgers, and this is None when syncing Audit and Pool.
For a better consistency it would make sense to set it for every ledger.
It also would simplify a check at the beginning of _on_XXX_sycned
methods.
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.
Actually I hope to get rid of _current_ledger
once parallel catch-up is implemented, so I'm inclined to leave it as is for now.
self._state = self.State.SyncingOthers | ||
self._sync_next_ledger() | ||
|
||
def _on_other_synced(self, msg): |
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 really need 3 on_synced methods?
I think we could create just one generic method (like the current _on_other_synced
).
The only change is to set the state according to msg.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.
I agree that this looks like some code duplication, however I wanted to make state machine as explicit as possible. I'll think about improving this.
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
leecher = node.ledgerManager._node_leecher | ||
|
||
# TODO: It does ignore designated ledger order, however it will obsolete after implemnting parallel catchup | ||
ledger_ids = [ledger.id for ledger in node.ledgerManager.ledgerRegistry.values() |
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.
Why do we exclude Audit and Pool ledgers from checking?
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.
They are handled by separate states in NodeLeecherService, therefore they are not handled by get_next_ledger. Actually this function (and test) will be removed once parallel catch-up is implemented.
Signed-off-by: Sergey Khoroshavin sergey.khoroshavin@dsr-corporation.com