IGNITE-13508: Test scenario of two-phased rebalance #8385

Sega76 · 2020-10-21T16:26:28Z

Thank you for submitting the pull request to the Apache Ignite.

In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:

The Contribution Checklist

There is a single JIRA ticket related to the pull request.
The web-link to the pull request is attached to the JIRA ticket.
The JIRA ticket has the Patch Available state.
The pull request body describes changes that have been made.
The description explains WHAT and WHY was made instead of HOW.
The pull request title is treated as the final commit message.
The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
A reviewer has been mentioned through the JIRA comments
(see the Maintainers list)
The pull request has been checked by the Teamcity Bot and
the green visa attached to the JIRA ticket (see TC.Bot: Check PR)

Notes

If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.

ivandasch · 2021-03-02T11:07:31Z

modules/ducktests/tests/ignitetest/tests/two_phased_rebalanced_test.py

+
+        restart_with_clean_idx_node_on_cell(cells, [0, 1])
+
+        self.fix_pds_size(cells, "After rebalancing complate on nodes 0, 1. PDS.")


timoninmaxim · 2021-03-12T15:33:03Z

...ducktests/src/main/java/org/apache/ignite/internal/ducktest/tests/DeleteDataApplication.java

+
+        int size = jNode.get("size").asInt();
+
+        int bachSize = Optional.ofNullable(jNode.get("bachSize"))


s/bach/batch/

timoninmaxim · 2021-03-12T15:35:24Z

...ducktests/src/main/java/org/apache/ignite/internal/ducktest/tests/DeleteDataApplication.java

+        while (fromIdx < listSize) {
+            toIdx = Math.min(fromIdx + bachSize, listSize);
+
+            futures.add(cache.removeAllAsync(new TreeSet<>(keys.subList(fromIdx, toIdx))));


iteratro() supports dynamic remove. Can we use it to simplify deletion process?

removal in batches faster
37 seconds versus 351

timoninmaxim · 2021-03-12T15:40:57Z

modules/ducktests/tests/ignitetest/services/utils/ignite_configuration/cache.py

+    Affinity.
+    """
+    name: str = AFFINITY_BACKUP_FILTER
+    constructor_arg: str = CELL


Let's replace it for smth meaningful, e.g. attr_name

timoninmaxim · 2021-03-12T15:43:07Z

modules/ducktests/tests/ignitetest/services/utils/util.py

+import os
+
+
+def copy_file_to_dest(node, file_path: str, dest_dir: str):


You used this method only once, so there is no need to create a separate module for this method.

I think it will be useful in the future when we need to save some data for the result.
For example, to save statistics files

We will do it after the future comes. No need for a new module right now.

timoninmaxim · 2021-03-12T15:44:42Z

modules/ducktests/tests/ignitetest/tests/two_phased_rebalanced_test.py

+    Two-phase rebalancing test case.
+    """
+    # pylint: disable=R0914
+    @cluster(num_nodes=NUM_NODES_CELL * NUM_CELL + 2)


Do you change default IGNITE_NUM_CONTAINERS value to 18?

using NUM_CELL = 2

timoninmaxim · 2021-03-12T16:10:18Z

modules/ducktests/tests/ignitetest/tests/two_phased_rebalanced_test.py

+        Test case of two-phase rebalancing.
+        Preparations.
+            1. Start 4 cells.
+            2. Load data to cache with the mentioned above affinity function and fix PDS size on all nodes.


Let's replace "fix" with smth different word. You use it in the meaning of "limit" [1]. But actually, you do not limit anything, but store an actual value of PDS size for future comparison.

[1] https://www.ldoceonline.com/dictionary/fix

timoninmaxim · 2021-03-12T16:15:41Z

modules/ducktests/tests/ignitetest/tests/two_phased_rebalanced_test.py

+    return copy_file_to_dest(node, dump, log_dir)
+
+
+def start_cell(test_context, config: IgniteConfiguration, num_nodes: int, jvm_opts: list) -> IgniteService:


Actually I don't think that this method improve readability. Maybe you can put it within the start_cells function without docs?

timoninmaxim · 2021-03-12T16:20:46Z

modules/ducktests/tests/ignitetest/tests/two_phased_rebalanced_test.py

+        :param msg Information message.
+        :return dict with hostname -> pds size in megabytes.
+        """
+        assert len(cells) > 0


Useless assert

timoninmaxim · 2021-03-12T16:21:21Z

modules/ducktests/tests/ignitetest/tests/two_phased_rebalanced_test.py

+        size = len(cell.nodes)
+
+        for i in idxs:
+            assert i < size


Useless assert here and below

timoninmaxim · 2021-03-12T16:22:01Z

modules/ducktests/tests/ignitetest/tests/two_phased_rebalanced_test.py

+        try:
+            IgniteAwareService.await_event_on_node('Skipping checkpoint', node, timeout_sec=60)
+        except errors.TimeoutError as ex:
+            self.logger.warn(ex)


Why do you ignore this error?

it may not happen after start waiting

timoninmaxim · 2021-03-15T16:14:30Z

...ducktests/src/main/java/org/apache/ignite/internal/ducktest/tests/DeleteDataApplication.java

+
+        int size = jNode.get("size").asInt();
+
+        int batchSize = jNode.get("batchSize").asInt();


Provide default value for batch size. As it runs to infinite loop with default 0 value.

timoninmaxim · 2021-03-15T17:07:50Z

...ducktests/src/main/java/org/apache/ignite/internal/ducktest/tests/DeleteDataApplication.java

+        int fromIdx = 0;
+        int toIdx = 0;
+
+        while (fromIdx < listSize) {


for (int from = 0; from < listSize; from += batchSize)

timoninmaxim · 2021-03-15T17:10:12Z

...ducktests/src/main/java/org/apache/ignite/internal/ducktest/tests/DeleteDataApplication.java

+            fromIdx = toIdx;
+        }
+
+        futures.forEach(IgniteFuture::get);


Try do not use Future.get without specified timeout. We can hang, AFAIK timeout is optional parameter for IgniteApplicationService?

timoninmaxim · 2021-03-15T17:18:51Z

modules/ducktests/tests/ignitetest/tests/two_phased_rebalanced_test.py

+                    "valueSizeKb": 1}
+        )
+        app.run()
+        app.free()


Could you please provide a ref to implementation of the free() method? I can't find one.

ducktape/services/service.py
def free(self):
"""Free each node. This 'deallocates' the nodes so the cluster can assign them to other services."""
for node in self.nodes:
self.logger.info("%s: freeing node" % self.who_am_i(node))
node.account.logger = None
self.cluster.free(node)

self.nodes = []

Yeah, I saw this code. But didn't see cluster.free.

ducktape/cluster/cluster.py
def free(self, nodes):
"""Free the given node or list of nodes"""
if isinstance(nodes, collections.Iterable):
for s in nodes:
self.free_single(s)
else:
self.free_single(nodes)

I saw this code, I was looking for this code:

https://github.com/confluentinc/ducktape/blob/master/ducktape/cluster/json.py#L122

I'm OK then.

timoninmaxim · 2021-03-15T17:25:38Z

modules/ducktests/tests/ignitetest/tests/two_phased_rebalanced_test.py

+    # pylint: disable=R0914
+    @cluster(num_nodes=(NUM_NODES_CELL * NUM_CELL) + 1)
+    @ignite_versions(str(DEV_BRANCH), str(LATEST_2_9), str(LATEST_2_8))
+    def two_phased_rebalancing_test(self, ignite_version):


WDYT if we add some tests that check concurrent cases? Smth like rebalance while transaction with batch update/delete is open on other node in a cell? Is it a valid case to check it there?

Agreed that there is no other cases.

timoninmaxim · 2021-03-18T07:19:31Z

modules/ducktests/tests/ignitetest/services/utils/ignite_aware.py

+        """
+        delta_time = datetime.now() + timedelta(seconds=timeout_sec)
+
+        rebalanced = False


Unuseful var. There is already rebalanced var declared on the 475 line. If self.nodes is empty then code raises TimeoutError, but it is actually wrong. Please replace it with logic that shold handle empty nodes list if it is required.

This is an announcement of a variable in order to expand the zone of its visibility.
added
assert self.nodes, 'Node list is empty.'

timoninmaxim · 2021-03-18T07:23:49Z

modules/ducktests/tests/ignitetest/tests/two_phased_rebalanced_test.py

+                                         indexed_types=['java.lang.Long', 'byte[]'])],
+                                     metric_exporter='org.apache.ignite.spi.metric.jmx.JmxMetricExporterSpi')
+
+        cluster_size = len(self.test_context.cluster)


How does self.test_context.cluster relate to a value of @cluster annotation?

We can set the cluster size through globals and then more cells will be used in the test

# Conflicts: # modules/ducktests/tests/ignitetest/services/utils/ignite_aware.py

Sega76 force-pushed the IGNITE-13508 branch 8 times, most recently from 62c329d to 6789b34 Compare February 22, 2021 12:44

IGNITE-13508: added two-phase rebalance test

08c0c1a

Sega76 force-pushed the IGNITE-13508 branch 3 times, most recently from 33edec3 to edccd69 Compare February 24, 2021 14:41

IGNITE-13508: fix

600c0ba

Sega76 force-pushed the IGNITE-13508 branch 3 times, most recently from 9d43b8d to ca16b89 Compare February 24, 2021 16:35

IGNITE-13508: fix

ecf4b1d

Sega76 force-pushed the IGNITE-13508 branch from ca16b89 to ecf4b1d Compare February 25, 2021 08:42

Sega76 added 3 commits February 25, 2021 12:59

IGNITE-13508: fix pydoc

9a642b2

IGNITE-13508: fix pydoc

7257a29

IGNITE-13508: fix comments

df0aef6

ivandasch reviewed Mar 2, 2021

View reviewed changes

Sega76 force-pushed the IGNITE-13508 branch 2 times, most recently from 3c52db2 to f864c1e Compare March 4, 2021 07:53

IGNITE-13508: fix comments

233943f

Sega76 force-pushed the IGNITE-13508 branch from f864c1e to 233943f Compare March 4, 2021 10:17

Sega76 added 2 commits March 10, 2021 00:19

IGNITE-13508: added indexes

5eb455b

IGNITE-13508: added await_cluster_idle after rebalance

46ce2eb

timoninmaxim requested changes Mar 12, 2021

View reviewed changes

IGNITE-13508: fix comments

3a9ef2b

timoninmaxim reviewed Mar 15, 2021

View reviewed changes

IGNITE-13508: added use globals cluster_size

33562ed

timoninmaxim reviewed Mar 15, 2021

View reviewed changes

Sega76 force-pushed the IGNITE-13508 branch from db232e2 to 8c6b432 Compare March 16, 2021 07:08

IGNITE-13508: fix comments

0290e1c

Sega76 force-pushed the IGNITE-13508 branch from 8c6b432 to 0290e1c Compare March 16, 2021 09:00

timoninmaxim reviewed Mar 18, 2021

View reviewed changes

Sega76 added 6 commits March 18, 2021 11:26

IGNITE-13508: fix comments

2d2b88e

IGNITE-13508: fix comments

1f396d2

IGNITE-13508: fix comments

987caba

Merge remote-tracking branch 'ignite/ignite-ducktape' into IGNITE-13508

c617c3a

IGNITE-13508: fix comments

6234090

IGNITE-13508: fix comments

78df8d1

Sega76 force-pushed the IGNITE-13508 branch 4 times, most recently from ad45093 to 68ef1a9 Compare March 23, 2021 10:56

timoninmaxim approved these changes Mar 23, 2021

View reviewed changes

IGNITE-13508: fix versions

ca3976e

Sega76 force-pushed the IGNITE-13508 branch from 68ef1a9 to ca3976e Compare March 24, 2021 08:18

Sega76 added 2 commits March 25, 2021 13:44

Merge remote-tracking branch 'ignite/ignite-ducktape' into IGNITE-13508

0dd8561

IGNITE-13508: remove LATEST_2_9

31de1d4

Sega76 force-pushed the IGNITE-13508 branch from 9176241 to 31de1d4 Compare March 25, 2021 20:34

Merge remote-tracking branch 'ignite/ignite-ducktape' into IGNITE-13508

d7788a1

# Conflicts: # modules/ducktests/tests/ignitetest/services/utils/ignite_aware.py

anton-vinogradov deleted the branch apache:ignite-ducktape September 6, 2021 08:14

anton-vinogradov closed this Sep 6, 2021


		restart_with_clean_idx_node_on_cell(cells, [0, 1])

		self.fix_pds_size(cells, "After rebalancing complate on nodes 0, 1. PDS.")


		int size = jNode.get("size").asInt();

		int bachSize = Optional.ofNullable(jNode.get("bachSize"))

		import os


		def copy_file_to_dest(node, file_path: str, dest_dir: str):

		return copy_file_to_dest(node, dump, log_dir)


		def start_cell(test_context, config: IgniteConfiguration, num_nodes: int, jvm_opts: list) -> IgniteService:


		int size = jNode.get("size").asInt();

		int batchSize = jNode.get("batchSize").asInt();

IGNITE-13508: Test scenario of two-phased rebalance #8385

IGNITE-13508: Test scenario of two-phased rebalance #8385

Conversation

Sega76 commented Oct 21, 2020 • edited

The Contribution Checklist

Notes

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

Choose a reason for hiding this comment

Sega76 Mar 15, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sega76 commented Oct 21, 2020 •

edited

Sega76 Mar 15, 2021 •

edited