INDY-1680: add function for removing replica #912

Toktar · 2018-09-07T14:46:57Z

Changes:

add function for remove a replica (and cleaning client requests for this replica)
restore the number of replicas during the view change
add tests for check this function and check the correct work of other systems
correctly process messages for switched off replicas
change a replicas list format in node.py to a SortedDict
change a replicas list format in monitors to a dict

Changes: - add function to remove a replica (and cleaning client requests for this replica) - add grow count of replicas in view change - add tests for check this function and check the correct work of other systems TODO: - correctly process messages for switched off replicas (for implementation this replicas should be a dict) Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Changes: - correctly process messages for switched off replicas (for implementation this replicas should be a dict) - change a replicas list format to a dict Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

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

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

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

Changes: - add method add_replica - add method remove_replica - rename method to restoreReplicas - adjustReplicas(from, to) - change monitor replicas list to dict - in _select_primaries add restore_replicas - in restore_replicas in remove requests use requestQueues Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

…nto task-1680-switch-of-replica

spivachuk · 2018-09-13T09:02:19Z

plenum/recorder/replayable_node.py

@@ -87,7 +87,7 @@ def create_replicas(self, config=None):
            return _TestReplicas(self, self.monitor, config)

        def adjustReplicas(self):


Why does this method violate the base class contract?
Also the logic of this method should be corrected since replicas is a dict now.

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

spivachuk · 2018-09-13T09:17:13Z

plenum/server/instances.py


    @property
    def masterId(self) -> Optional[int]:
        """
        Return the index of the replica that belongs to the master protocol
        instance
        """
-        return 0 if self.count > 0 else None
+        return 0 if len(self.started) > 0 else None


Correct the condition.

spivachuk · 2018-09-13T09:49:08Z

plenum/server/monitor.py

-    def removeInstance(self, index=None):
-        # TODO: This doesn't take into account index, but this function is never called with defined index,
-        # probably we can simplify this thing?
+    def removeInstance(self, index):


Please correct the parameter name.

Done. And in Instances.remove() changed parameter name too.

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

spivachuk · 2018-09-13T09:56:59Z

plenum/server/node.py

-        while len(self.replicas) > self.requiredNumberOfInstances:
-            self.replicas.shrink()
+            self.processStashedMsgsForReplica(old_required_number_of_instances)
+            old_required_number_of_instances += 1


The parameter name does not suppose the value changes.

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

spivachuk · 2018-09-13T11:07:51Z

plenum/server/replicas.py

@@ -73,15 +76,17 @@ def _master_replica(self):

    def service_inboxes(self, limit: int = None):
        number_of_processed_messages = \
-            sum(replica.serviceQueues(limit) for replica in self._replicas)
+            sum(replica.serviceQueues(limit) for replica in self._replicas.values())
        return number_of_processed_messages

    def pass_message(self, message, instance_id=None):


Also Node.msgHasAcceptableInstId method should be corrected. It must stash messages for instances with a number higher than the current maximum required number.

spivachuk · 2018-09-13T11:16:55Z

plenum/server/replicas.py

+    def primaries(self) -> dict:
+        primaries = dict()
+        for r in self._replicas.values():
+            primaries[r.instId] = r


This property must return not replicas but names of instances primaries.

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

plenum/test/monitoring/test_request_time_tracker.py

spivachuk · 2018-09-13T14:26:37Z

plenum/test/test_node.py

@@ -145,7 +145,7 @@ def __init__(self, *args, **kwargs):
            notifierEventTriggeringConfig=notifierEventTriggeringConfig,
            pluginPaths=pluginPaths)
        for i in range(len(self.replicas)):


It would be more clear to iterate over the keys.

spivachuk · 2018-09-13T14:29:50Z

plenum/server/replicas.py

+        if index is None:
+            index = self.num_replicas - 1
+        replica = self._replicas.pop(index)
+        for request_key in replica.requestQueues:


requestQueues is not a request queue, it is a map from ledger IDs to request queues.
Is there a unit test verifying freeing requests?

Fixed it.
Now all tests from test_replica_removing.py check it.

It makes sense to add verification of freeing the request by the replica (i.e. forwardedTo has been decreased by 1) to test_replica_removing_in_ordering.

spivachuk · 2018-09-13T14:59:01Z

plenum/test/replica/test_replica_removing.py

+    ensureElectionsDone(looper=looper, nodes=txnPoolNodeSet)
+
+
+def _check_replica_removed(node, start_replicas_count):


This method should take an instance ID. Also it should check that items related to the replica are removed from all the relevant collections and it should not verify conditions not related to removing the replica (such as master degradation and requests freeing).

Added check that replica removed from all relevant collections and changed parameters list. But I think, we should check things as a master degraded in _check_replica_removed() because it's a proof that replica was successfully removed without negative effects.

We should check this but not in _check_replica_removed method since it is not its responsibility according to its name.
Also assert len(node.requests) == 0 should not be a responsibility of this method too.

spivachuk · 2018-09-13T15:04:22Z

plenum/test/replica/test_replica_removing.py

+    index = start_replicas_count - 1
+    node.replicas.remove_replica(index)
+    _check_replica_removed(node, start_replicas_count)
+    # trigger view change on all nodes


Such the teardown sections in tests make them dependent on each other. Instead of this introduce a fixture which ensures view change and use this fixture in every test.

spivachuk · 2018-09-13T15:20:17Z

plenum/test/replica/test_replica_removing.py

+    looper.run(checkNodesConnected(txnPoolNodeSet))
+    waitForViewChange(looper, txnPoolNodeSet, expectedViewNo=1,
+                      customTimeout=2 * tconf.VIEW_CHANGE_TIMEOUT)
+    ensureElectionsDone(looper=looper, nodes=txnPoolNodeSet)


Restoration of replicas should be verified in an obvious way.

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

spivachuk · 2018-09-14T11:21:23Z

plenum/recorder/replayable_node.py

+                           old_required_number_of_instances: int,
+                           new_required_number_of_instances: int):
+            r = super().adjustReplicas(old_required_number_of_instances,
+                                       new_required_number_of_instances)
            if r > 0:
                if hasattr(self, 'sent_pps'):
                    new_replicas = self.replicas[-r:]


self.replicas is a dict now. Correct its usage here.

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

spivachuk · 2018-09-14T16:10:35Z

plenum/recorder/replayable_node.py

-                    new_replicas = self.replicas[-r:]
+                    new_replicas = [r for inst_id, r in self.replicas
+                                    if old_required_number_of_instances >=
+                                    inst_id < new_required_number_of_instances]


Fix the inequation.

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

changes: - updated test_request_time_tracker.py - updated test_replica_removing_before_ordering() Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

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

Toktar added 2 commits September 7, 2018 16:43

INDY-1680: change a replicas list format to a dict

1c9f5ed

Changes: - correctly process messages for switched off replicas (for implementation this replicas should be a dict) - change a replicas list format to a dict Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

Toktar changed the title ~~INDY-1680: added function for removing replica~~ INDY-1680: add function for removing replica Sep 7, 2018

Toktar added 9 commits September 10, 2018 12:31

INDY-1680: divide method shrink to shrink() and remove_replica()

d83ee0b

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

INDY-1680: fix problem with a replica removing

4726140

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

INDY-1680: fix test_set_H_to_maxsize_and_not_stash_on_backup.py

4a5f86d

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

INDY-1680: divide adjustReplicas to adjustReplicas() and update_repli…

5cb188e

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

INDY-1680: bugfix in adjustReplicas()

ae39a5c

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

INDY-1680: change replicas iterator

91b0703

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

INDY-1680: bugfix for replicas iterator usages

9579fe9

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

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

c0aa049

…nto task-1680-switch-of-replica

spivachuk reviewed Sep 13, 2018

View reviewed changes

INDY-1680: update tests

7e91ecd

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

spivachuk reviewed Sep 13, 2018

View reviewed changes

INDY-1680: Monitor refactoring

fcb700c

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

spivachuk reviewed Sep 13, 2018

View reviewed changes

Toktar added 2 commits September 13, 2018 13:10

INDY-1680: Refactoring method adjustReplicas()

25c4c4a

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

INDY-1680: Bugfix in method adjustReplicas()

1525664

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

spivachuk reviewed Sep 13, 2018

View reviewed changes

INDY-1680: bugfix (change stashing logic) in msgHasAcceptableInstId()

453dec3

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

spivachuk reviewed Sep 13, 2018

View reviewed changes

plenum/test/monitoring/test_request_time_tracker.py Show resolved Hide resolved

spivachuk reviewed Sep 13, 2018

View reviewed changes

INDY-1680: Fix tests and request removing

8d9a03d

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

INDY-1680: Fix usages of Replicas.__len__

fdaab40

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

spivachuk reviewed Sep 14, 2018

View reviewed changes

INDY-1680: Fix problem with usage of Instances.count

901a40f

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

spivachuk reviewed Sep 14, 2018

View reviewed changes

Toktar added 3 commits September 17, 2018 15:59

INDY-1680: Fix recorder adjustReplicas() and refactoring tests

a483266

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

INDY-1680: update tests

692599c

changes: - updated test_request_time_tracker.py - updated test_replica_removing_before_ordering() Signed-off-by: toktar <renata.toktar@dsr-corporation.com>

INDY-1680: update tests, change replicas to SortedDict

85c0aa8

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

spivachuk approved these changes Sep 18, 2018

View reviewed changes

andkononykhin approved these changes Sep 18, 2018

View reviewed changes

andkononykhin merged commit 2e1b95f into hyperledger:master Sep 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INDY-1680: add function for removing replica #912

INDY-1680: add function for removing replica #912

Toktar commented Sep 7, 2018 •

edited

spivachuk Sep 13, 2018 •

edited

Toktar Sep 13, 2018

spivachuk Sep 13, 2018

Toktar Sep 13, 2018

spivachuk Sep 13, 2018

Toktar Sep 13, 2018

spivachuk Sep 13, 2018

Toktar Sep 13, 2018

spivachuk Sep 13, 2018 •

edited

Toktar Sep 14, 2018

spivachuk Sep 13, 2018

Toktar Sep 14, 2018

spivachuk Sep 13, 2018

Toktar Sep 14, 2018

spivachuk Sep 13, 2018 •

edited

Toktar Sep 14, 2018

spivachuk Sep 14, 2018 •

edited

spivachuk Sep 13, 2018

Toktar Sep 14, 2018

spivachuk Sep 14, 2018 •

edited

spivachuk Sep 13, 2018

Toktar Sep 14, 2018

spivachuk Sep 13, 2018

Toktar Sep 14, 2018

spivachuk Sep 14, 2018

Toktar Sep 14, 2018

spivachuk Sep 14, 2018 •

edited

		@@ -87,7 +87,7 @@ def create_replicas(self, config=None):
		return _TestReplicas(self, self.monitor, config)

		def adjustReplicas(self):

		ensureElectionsDone(looper=looper, nodes=txnPoolNodeSet)


		def _check_replica_removed(node, start_replicas_count):

INDY-1680: add function for removing replica #912

INDY-1680: add function for removing replica #912

Conversation

Toktar commented Sep 7, 2018 • edited

spivachuk Sep 13, 2018 • edited

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

spivachuk Sep 13, 2018 • edited

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

spivachuk Sep 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spivachuk Sep 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spivachuk Sep 14, 2018 • edited

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

spivachuk Sep 14, 2018 • edited

Choose a reason for hiding this comment

Toktar commented Sep 7, 2018 •

edited

spivachuk Sep 13, 2018 •

edited

spivachuk Sep 13, 2018 •

edited

spivachuk Sep 13, 2018 •

edited

spivachuk Sep 14, 2018 •

edited

spivachuk Sep 14, 2018 •

edited

spivachuk Sep 14, 2018 •

edited