INDY-1386: clear requests list after view change for ordered txns #781

Toktar · 2018-06-29T08:24:16Z

Changes:

add tests in test_different_last_ordered_before_view_change.py,
clear requests after view change for ordered txns,
change last ordered after view change to maximum of ordered on master and current value.

Changes: - add test ordering on master with view change. - add test ordering on backup with view change. Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Changes: - add test test_different_prepares_on_backup_before_view_change - clear requests after view change - change last ordered to max ordered after view change Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Toktar · 2018-06-29T08:33:49Z

plenum/server/replica.py

+        master_last_ordered_3pc = self.node.master_replica.last_ordered_3pc
+        if master_last_ordered_3pc[0] >= self.last_ordered_3pc[0] \
+                and master_last_ordered_3pc[1] > self.last_ordered_3pc[1]:
+            self.last_ordered_3pc = master_last_ordered_3pc


We store here the value of the tuple, without the risk of passing the reference, is it?

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

ashcherbakov · 2018-06-29T09:33:22Z

plenum/server/view_change/view_changer.py

@@ -530,6 +530,8 @@ def startViewChange(self, proposed_view_no: int):

        self.node.on_view_change_start()
        self.node.start_catchup()
+        for replica in self.node.replicas:


Should we do it when we finished view changed?

ashcherbakov · 2018-06-29T10:28:53Z

plenum/server/replica.py

+        master_last_ordered_3pc = self.node.master_replica.last_ordered_3pc
+        if master_last_ordered_3pc[0] >= self.last_ordered_3pc[0] \
+                and master_last_ordered_3pc[1] > self.last_ordered_3pc[1]:
+            self.last_ordered_3pc = master_last_ordered_3pc


Do we need to set last_ordered_3pc here?

We can to do it in other place. Where will it be better?

ashcherbakov · 2018-06-29T10:29:40Z

plenum/server/replica.py

+            self.requests.free(key)
+            self.requestQueues[int(ledger_id)].discard(key)
+        master_last_ordered_3pc = self.node.master_replica.last_ordered_3pc
+        if master_last_ordered_3pc[0] >= self.last_ordered_3pc[0] \


Please use compare_3PC_keys for comparison

ashcherbakov · 2018-06-29T10:35:13Z

plenum/test/view_change/test_different_last_ordered_before_view_change.py

+                       fast_nodes,
+                       (old_view_no, old_last_ordered[1] + 1),
+                       slow_instance))
+        last_ordered_for_slow = slow_nodes[0].replicas[slow_instance].last_ordered_3pc


Please also check that last_ordered on slow nodes (on slow instance) is (old_view_no, old_last_ordered).
Please check that last_ordered on all master instances is (old_view_no, old_last_ordered+1).

On backup instances last ordered is (new_view_no, 1) because with current logic we sending preprepare message without transactions. And order it. I think it's a very strange behavior. But for backup, may be it is normal.
Added check that last_ordered on all master instances is (old_view_no, old_last_ordered+1).

Need to add a check of empty transaction list for sending preprepare?

ashcherbakov · 2018-06-29T10:40:48Z

plenum/test/view_change/test_different_last_ordered_before_view_change.py

+        assert node.replicas[instId].last_ordered_3pc == last_ordered
+
+
+def view_change_done(nodes: [Node], view_no):


This doesn't indicate end of view change, rather the beginning.
Please call ensureElectionsDone in addition to make sure that it's finished.

ashcherbakov · 2018-06-29T10:45:09Z

plenum/test/view_change/test_different_last_ordered_before_view_change.py

+from plenum.test.stasher import delay_rules
+from stp_core.loop.eventually import eventually
+
+


Please add tests that make sure we don't request any Propagates after View Change if one of the replicas on backup instance ordered less than others, and it becomes a Primary

Added check that no one propagate request wasn't sent.

ashcherbakov · 2018-06-29T10:47:45Z

plenum/test/view_change/test_different_last_ordered_before_view_change.py

+            node.view_changer.on_master_degradation()
+
+        # wait for view change done on all nodes
+        looper.run(eventually(view_change_done, txnPoolNodeSet, old_view_no + 1))


This doesn't indicate end of view change, rather the beginning.
Please call ensureElectionsDone in addition to make sure that it's finished.

Changes: - update test_different_prepares_on_backup_before_view_change - move requests cleaning after view change Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Changes: - added check in test_different_last_ordered_on_backup_before_view_change for last_order on master instance. Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

ashcherbakov · 2018-07-03T09:25:25Z

plenum/server/node.py

@@ -596,6 +596,8 @@ def on_view_change_complete(self):
        if self.view_changer.propagate_primary:  # TODO VCH
            self.master_replica.on_propagate_primary_done()
        self.view_changer.last_completed_view_no = self.view_changer.view_no
+        for replica in self.replicas:


Please add a comment on why we do it

Should we describe the case with propagates or commented what we do?

Yes, please describe the case with propagates (a reference to the tests would also be useful)

ashcherbakov · 2018-07-03T09:29:46Z

plenum/server/replica.py

                                                      pp_seq_no)
-        if not resp:
+        if not (validReqs or inValidReqs):


Please add a unit test for this (see test/replica/test_api)

Are we need to check that create3PCBatch() doesn't return value or log message too (and change logLevel )?

ashcherbakov · 2018-07-03T10:14:40Z

plenum/test/view_change/test_different_last_ordered_before_view_change.py

+from stp_core.loop.eventually import eventually
+
+
+def test_different_last_ordered_on_backup_before_view_change(looper, txnPoolNodeSet,


I would rename the tests to test_no_propagate_request_on_different_last_ordered_on_backup_before_vc

ashcherbakov · 2018-07-03T10:17:04Z

plenum/test/view_change/test_different_last_ordered_before_view_change.py

+               for node in txnPoolNodeSet)
+
+
+def test_different_prepares_on_backup_before_view_change(looper, txnPoolNodeSet,


I would rename the tests to test_no_propagate_request_on_different_prepares_on_backup_before_vc

ashcherbakov · 2018-07-03T10:22:18Z

plenum/test/view_change/test_different_last_ordered_before_view_change.py

+               for node in txnPoolNodeSet)
+
+
+def test_different_last_ordered_on_master_before_view_change(looper, txnPoolNodeSet,


I would rename the tests to test_no_propagate_request_on_different_last_ordered_on_master_before_vc

ashcherbakov · 2018-07-03T10:23:01Z

plenum/test/view_change/test_different_last_ordered_before_view_change.py

+               (node.viewNo, ppSeqNo) in replica.sentPrePrepares
+
+
+def last_ordered(nodes: [Node],


Please rename to check_last_ordered

…_before_vc.py Changes: - update test_no_propagate_request_on_different_last_ordered_before_vc.py - added unit test_create_3pc_batch_with_empty_requests Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

…nto task-1386-limit-propagate-request

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

…nto task-1386-limit-propagate-request

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Toktar · 2018-07-04T13:16:59Z

test this please

…ests() Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Toktar added 2 commits June 28, 2018 15:35

INDY-1386: add test to detect ordering with view change.

a1fe78f

Changes: - add test ordering on master with view change. - add test ordering on backup with view change. Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1386: clear requests after view change

3e5d051

Changes: - add test test_different_prepares_on_backup_before_view_change - clear requests after view change - change last ordered to max ordered after view change Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Toktar commented Jun 29, 2018

View reviewed changes

INDY-1386: FakeNode added attribute requests

ced61d3

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

ashcherbakov requested changes Jun 29, 2018

View reviewed changes

Toktar added 3 commits July 3, 2018 10:43

INDY-1386: move requests cleaning

396175c

Changes: - update test_different_prepares_on_backup_before_view_change - move requests cleaning after view change Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1386: added check in test_different_last_ordered_before_view_change

356c504

Changes: - added check in test_different_last_ordered_on_backup_before_view_change for last_order on master instance. Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1386: add check for empty preprepare requests list

0ee8597

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

ashcherbakov reviewed Jul 3, 2018

View reviewed changes

Toktar added 6 commits July 3, 2018 13:51

INDY-1386: update test_no_propagate_request_on_different_last_ordered…

c7a4c19

…_before_vc.py Changes: - update test_no_propagate_request_on_different_last_ordered_before_vc.py - added unit test_create_3pc_batch_with_empty_requests Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Merge branch 'master' of https://github.com/hyperledger/indy-plenum i…

5ab1778

…nto task-1386-limit-propagate-request

INDY-1386: add comment to cleaning requests list in view change.

86167b7

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Merge branch 'master' of https://github.com/hyperledger/indy-plenum i…

bf3deb6

…nto task-1386-limit-propagate-request

INDY-1386: don't change last ordered for new node in view change

8209557

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1386: fix test test_create_3pc_batch_with_empty_requests()

7993976

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1386: fix stateRootHash in test_create_3pc_batch_with_empty_requ…

c132d64

…ests() Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

ashcherbakov approved these changes Jul 5, 2018

View reviewed changes

ashcherbakov merged commit 6846b63 into hyperledger:master Jul 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INDY-1386: clear requests list after view change for ordered txns #781

INDY-1386: clear requests list after view change for ordered txns #781

Toktar commented Jun 29, 2018

Toktar Jun 29, 2018

ashcherbakov Jun 29, 2018

Toktar Jul 3, 2018

ashcherbakov Jun 29, 2018

Toktar Jun 29, 2018

ashcherbakov Jun 29, 2018

Toktar Jul 3, 2018

ashcherbakov Jun 29, 2018

Toktar Jul 3, 2018

Toktar Jul 3, 2018

ashcherbakov Jun 29, 2018

Toktar Jul 3, 2018

ashcherbakov Jun 29, 2018

Toktar Jul 3, 2018

ashcherbakov Jun 29, 2018

Toktar Jul 3, 2018

ashcherbakov Jul 3, 2018

Toktar Jul 3, 2018

ashcherbakov Jul 4, 2018

ashcherbakov Jul 3, 2018

Toktar Jul 3, 2018

ashcherbakov Jul 3, 2018

Toktar Jul 3, 2018

ashcherbakov Jul 3, 2018

Toktar Jul 3, 2018

ashcherbakov Jul 3, 2018

Toktar Jul 3, 2018

ashcherbakov Jul 3, 2018

Toktar Jul 3, 2018

Toktar commented Jul 4, 2018

		assert node.replicas[instId].last_ordered_3pc == last_ordered


		def view_change_done(nodes: [Node], view_no):

		from plenum.test.stasher import delay_rules
		from stp_core.loop.eventually import eventually

		from stp_core.loop.eventually import eventually


		def test_different_last_ordered_on_backup_before_view_change(looper, txnPoolNodeSet,

		for node in txnPoolNodeSet)


		def test_different_prepares_on_backup_before_view_change(looper, txnPoolNodeSet,

		for node in txnPoolNodeSet)


		def test_different_last_ordered_on_master_before_view_change(looper, txnPoolNodeSet,

		(node.viewNo, ppSeqNo) in replica.sentPrePrepares


		def last_ordered(nodes: [Node],

INDY-1386: clear requests list after view change for ordered txns #781

INDY-1386: clear requests list after view change for ordered txns #781

Conversation

Toktar commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toktar commented Jul 4, 2018