INDY-2140: Get rid of legacy view change completion check #1371
Conversation
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>
skhoroshavin
requested review from
lampkin-diet,
ashcherbakov and
Toktar
as code owners
|
This pull request introduces 3 alerts and fixes 12 when merging ac6946e into cd8d150 - view on LGTM.com new alerts:
fixed alerts:
|
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
|
This pull request introduces 1 alert and fixes 11 when merging 45617f6 into de1f126 - view on LGTM.com new alerts:
fixed alerts:
|
| @@ -301,7 +301,11 @@ def _finish_view_change(self): | |||
| self.last_completed_view_no = self._data.view_no | |||
|
|
|||
| def _propose_view_change(self, suspision_code): | |||
| proposed_view_no = self._data.view_no + 1 | |||
| proposed_view_no = self._data.view_no | |||
| # TODO: For some reason not incrementing view_no in most cases leads to lots of failing/flaky tests | |||
There was a problem hiding this comment.
I didn't get this TODO comment... The only situation when we don't need to increment viewNo is STATE_SIGS_ARE_NOT_UPDATED code. So, what is the issue?
There was a problem hiding this comment.
It was ported from original code in old ViewChanger. Some time ago (last year AFAIR) we had a theory that during view change we shouldn't send IC with future view no unless it is a view change timeout itself. For some reason that didn't work, at that time we gave up and decided to just leave a comment for a better time. I'm just preserving it.
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
skhoroshavin
changed the title
|
There is high chance that after last commit all tests will pass, however I would still refrain from merging this (unless we find it critical enough to merge ASAP) because some edge cases of view change now take twice as much time (hence increase in timeouts in tests) which doesn't look healthy at all. |
|
This pull request introduces 1 alert and fixes 11 when merging 1cb586a into d3ee8dd - view on LGTM.com new alerts:
fixed alerts:
|
Signed-off-by: Sergey Khoroshavin <sergey.khoroshavin@dsr-corporation.com>
|
This pull request introduces 1 alert and fixes 11 when merging 4e086fc into d3ee8dd - view on LGTM.com new alerts:
fixed alerts:
|
skhoroshavin
changed the title
No description provided.