You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "kgusakov (via GitHub)" <gi...@apache.org> on 2023/05/14 22:21:50 UTC
[GitHub] [ignite-3] kgusakov opened a new pull request, #2070: IGNITE-19405
kgusakov opened a new pull request, #2070:
URL: https://github.com/apache/ignite-3/pull/2070
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] kgusakov commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "kgusakov (via GitHub)" <gi...@apache.org>.
kgusakov commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1211488537
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngine.java:
##########
@@ -187,39 +187,47 @@ public CompletableFuture<Void> onUpdate(WatchEvent evt) {
if (zoneId == tableZoneId) {
TableConfiguration tableCfg = tables.get(tableView.name());
- byte[] assignmentsBytes = ((ExtendedTableConfiguration) tableCfg).assignments().value();
+ int tableId = tableCfg.id().value();
- List<Set<Assignment>> tableAssignments = assignmentsBytes == null
- ? Collections.emptyList()
- : ByteUtils.fromBytes(assignmentsBytes);
+ CompletableFuture<List<Set<Assignment>>> tableAssignmentsFut = tableAssignments(
+ metaStorageManager,
+ tableId, distributionZoneConfiguration.partitions().value());
- for (int part = 0; part < distributionZoneConfiguration.partitions().value(); part++) {
- int tableId = tableCfg.id().value();
+ tableAssignmentsFut.thenAccept(tbls -> {
- TablePartitionId replicaGrpId = new TablePartitionId(tableId, part);
+ for (int part = 0; part < distributionZoneConfiguration.partitions().value(); part++) {
- int replicas = distributionZoneConfiguration.replicas().value();
+ TablePartitionId replicaGrpId = new TablePartitionId(tableId, part);
- int partId = part;
+ int replicas = distributionZoneConfiguration.replicas().value();
- updatePendingAssignmentsKeys(
- tableView.name(),
- replicaGrpId,
- filteredDataNodes,
- replicas,
- evt.entryEvent().newEntry().revision(),
- metaStorageManager,
- part,
- tableAssignments.isEmpty() ? Collections.emptySet() : tableAssignments.get(part)
- ).exceptionally(e -> {
- LOG.error(
- "Exception on updating assignments for [table={}, partition={}]", e, tableView.name(),
- partId
- );
-
- return null;
- });
- }
+ int partId = part;
+
+ updatePendingAssignmentsKeys(
+ tableView.name(),
+ replicaGrpId,
+ filteredDataNodes,
+ replicas,
+ evt.entryEvent().newEntry().revision(),
+ metaStorageManager,
+ partId,
+ tbls.isEmpty() ? Collections.emptySet() : tbls.get(partId)
Review Comment:
Just a note - this code was here before my PR.
But anyway, at the moment, we have no places where we can drop 1 partition assignment:
- we have an atomic invoke, which will write all assignments or nothing
- rebalance logic can change the stable partition for 1 partition, but not removing it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] kgusakov commented on pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "kgusakov (via GitHub)" <gi...@apache.org>.
kgusakov commented on PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#issuecomment-1569682970
> Previously it was discussed that usage of DistributionZoneManager#topologyVersionedDataNodes method will be removed in this pr. So why it is not removed?
I checked, that's not so simple (not just remove the one method). It looks like we need to remove the logic from the PR, whichi introduced the method too and fix some tests. I created the separate issue for it https://issues.apache.org/jira/browse/IGNITE-19600
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] kgusakov commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "kgusakov (via GitHub)" <gi...@apache.org>.
kgusakov commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1210231112
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##########
@@ -1038,7 +1038,9 @@ public void testOneNodeRestartWithGap() throws InterruptedException {
/**
* Checks that the table created in cluster of 2 nodes, is recovered on a node after restart of this node.
*/
- @Test
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-19506 here we receive the empty nodes from baseline manager,
+ // but should get the nodes from metastore instead
+ @Disabled("https://issues.apache.org/jira/browse/IGNITE-19506")
Review Comment:
At the moment:
- createTableAsyncInternal doesn't call topologyVersionedDataNodes
- instead datanodes calculated onTableCreate event from baseline manager (while IGNITE-19425 is not implemented, I guess)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] kgusakov commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "kgusakov (via GitHub)" <gi...@apache.org>.
kgusakov commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1210220957
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1277,6 +1296,7 @@ private void dropTableLocally(long causalityToken, String name, int tblId, List<
private Set<Assignment> calculateAssignments(TableConfiguration tableCfg, int partNum) {
return AffinityUtils.calculateAssignmentForPartition(
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-19506 we must use distribution zone keys here
Review Comment:
Changed the number of issue to IGNITE-19425
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1214224098
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngine.java:
##########
@@ -271,9 +279,11 @@ private CompletableFuture<?> onUpdateReplicas(ConfigurationNotificationEvent<Int
int newReplicas = replicasCtx.newValue();
- byte[] assignmentsBytes = ((ExtendedTableConfiguration) tblCfg).assignments().value();
+ int tableId = tblCfg.id().value();
- List<Set<Assignment>> tableAssignments = ByteUtils.fromBytes(assignmentsBytes);
+ CompletableFuture<List<Set<Assignment>>> tableAssignmentsFut = tableAssignments(
Review Comment:
I would use formatting like this
```
CompletableFuture<List<Set<Assignment>>> tableAssignmentsFut = tableAssignments(
metaStorageManager,
tableId,
partCnt
);
```
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/RebalanceUtil.java:
##########
@@ -376,4 +381,51 @@ public static <T> Set<T> union(Set<T> op1, Set<T> op2) {
public static <T> Set<T> intersect(Set<T> op1, Set<T> op2) {
return op1.stream().filter(op2::contains).collect(Collectors.toSet());
}
+
+ /**
+ * Returns partition assignments from meta storage.
+ *
+ * @param metaStorageManager Meta storage manager.
+ * @param tableId Table id.
+ * @param partitionNumber Partition number.
+ * @return Future with partition assignments as a value.
+ */
+ public static CompletableFuture<Set<Assignment>> partitionAssignments(
+ MetaStorageManager metaStorageManager, int tableId, int partitionNumber) {
+ return metaStorageManager
+ .get(stablePartAssignmentsKey(new TablePartitionId(tableId, partitionNumber)))
+ .thenApply(e -> (e.value() == null) ? null : ByteUtils.fromBytes(e.value()));
+ }
+
+ /**
+ * Returns table assignments for all table partitions from meta storage.
+ *
+ * @param metaStorageManager Meta storage manager.
+ * @param tableId Table id.
+ * @param numberOfPartitions Number of partitions.
+ * @return Future with table assignments as a value.
+ */
+ public static CompletableFuture<List<Set<Assignment>>> tableAssignments(
Review Comment:
Let's use formatting like this
```
public static CompletableFuture<List<Set<Assignment>>> tableAssignments(
MetaStorageManager metaStorageManager,
int tableId,
int numberOfPartitions
) {
```
Also, do we really need this method to be public?
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngine.java:
##########
@@ -187,39 +187,47 @@ public CompletableFuture<Void> onUpdate(WatchEvent evt) {
if (zoneId == tableZoneId) {
TableConfiguration tableCfg = tables.get(tableView.name());
- byte[] assignmentsBytes = ((ExtendedTableConfiguration) tableCfg).assignments().value();
+ int tableId = tableCfg.id().value();
- List<Set<Assignment>> tableAssignments = assignmentsBytes == null
- ? Collections.emptyList()
- : ByteUtils.fromBytes(assignmentsBytes);
+ CompletableFuture<List<Set<Assignment>>> tableAssignmentsFut = tableAssignments(
Review Comment:
I would use formatting like this
```
CompletableFuture<List<Set<Assignment>>> tableAssignmentsFut = tableAssignments(
metaStorageManager,
tableId,
distributionZoneConfiguration.partitions().value()
);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] kgusakov commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "kgusakov (via GitHub)" <gi...@apache.org>.
kgusakov commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1210377009
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##########
@@ -1038,7 +1038,9 @@ public void testOneNodeRestartWithGap() throws InterruptedException {
/**
* Checks that the table created in cluster of 2 nodes, is recovered on a node after restart of this node.
*/
- @Test
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-19506 here we receive the empty nodes from baseline manager,
+ // but should get the nodes from metastore instead
+ @Disabled("https://issues.apache.org/jira/browse/IGNITE-19506")
Review Comment:
Yep, fixed, thanks a lot for the advice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sergeyuttsel commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1209958053
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##########
@@ -1038,7 +1038,9 @@ public void testOneNodeRestartWithGap() throws InterruptedException {
/**
* Checks that the table created in cluster of 2 nodes, is recovered on a node after restart of this node.
*/
- @Test
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-19506 here we receive the empty nodes from baseline manager,
+ // but should get the nodes from metastore instead
+ @Disabled("https://issues.apache.org/jira/browse/IGNITE-19506")
Review Comment:
The reason is on table creation `createTableAsyncInternal` invokes `distributionZoneManager.topologyVersionedDataNodes(zoneId, cmgTopology.version())` and recieves datanodes:[iinrt_troon_0] instead of [iinrt_troon_0, iinrt_troon_1]. I fixed the same issue in testOneNodeRestartWithGap by using `startNode(1);` instead of `startPartialNode(1, null);`
At the last refinement, we decided that we will use this solution, close the IGNITE-19408 ticket and I will remove this TODO in my next pr.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1277,6 +1296,7 @@ private void dropTableLocally(long causalityToken, String name, int tblId, List<
private Set<Assignment> calculateAssignments(TableConfiguration tableCfg, int partNum) {
return AffinityUtils.calculateAssignmentForPartition(
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-19506 we must use distribution zone keys here
Review Comment:
Why did you add this TODO? Actually we will not use data nodes from zone manager here when IGNITE-19506 will be implemented. We want to use data nodes from zone manager here when https://issues.apache.org/jira/browse/IGNITE-19425 will be implemented.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1210234818
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -544,16 +548,60 @@ private CompletableFuture<?> onTableCreate(ConfigurationNotificationEvent<TableV
}
try {
- return createTableLocally(
+ DistributionZoneView zone =
+ getZoneById(distributionZonesConfiguration, (ctx.newValue()).zoneId()).value();
+
+ List<Set<Assignment>> assignments = AffinityUtils.calculateAssignments(
+ baselineMgr.nodes().stream().map(ClusterNode::name).collect(toList()),
Review Comment:
I see that you've removed usage of `DistributionZoneManager#topologyVersionedDataNodes` in a further commit, so in general I'm ok with using `baselineMgr.nodes()` and adding to do
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1211650575
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/distribution/zones/ItDistributionZonesFilterTest.java:
##########
@@ -220,15 +199,15 @@ void testFilteredEmptyDataNodesDoNotTriggerRebalance() throws Exception {
TablePartitionId partId = new TablePartitionId(table.tableId(), 0);
// Table was created after both nodes was up, so there wasn't any rebalance.
- assertPendingStableAreNull(metaStorageManager, partId);
+ assertPendingsAreNull(metaStorageManager, partId);
Review Comment:
This method was needed to check there wasn't any rebalance at all, now we check only pending, is this correct to check like this the absence of a rebalance?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/distribution/zones/ItDistributionZonesFilterTest.java:
##########
@@ -126,14 +124,6 @@ void testFilteredDataNodesPropagatedToStable() throws Exception {
TablePartitionId partId = new TablePartitionId(table.tableId(), 0);
- assertValueInStorage(
Review Comment:
We somehow how to check that stable has been actually rewritten. Now we only check it at the end
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1210054539
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -544,16 +548,60 @@ private CompletableFuture<?> onTableCreate(ConfigurationNotificationEvent<TableV
}
try {
- return createTableLocally(
+ DistributionZoneView zone =
+ getZoneById(distributionZonesConfiguration, (ctx.newValue()).zoneId()).value();
+
+ List<Set<Assignment>> assignments = AffinityUtils.calculateAssignments(
+ baselineMgr.nodes().stream().map(ClusterNode::name).collect(toList()),
Review Comment:
I think that we shouldn't use here baseline manger, what do you think about `DistributionZoneManager#topologyVersionedDataNodes` while proper solution with causality token is not implemented?
In any cases, we need to add todo here to use proper method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] kgusakov commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "kgusakov (via GitHub)" <gi...@apache.org>.
kgusakov commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1211499140
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -574,63 +623,38 @@ private CompletableFuture<?> onTableDelete(ConfigurationNotificationEvent<TableV
}
try {
+ int partitions =
+ getZoneById(distributionZonesConfiguration, ctx.oldValue().zoneId()).value().partitions();
+
dropTableLocally(
ctx.storageRevision(),
ctx.oldValue().name(),
ctx.oldValue().id(),
- ByteUtils.fromBytes(ctx.oldValue(ExtendedTableView.class).assignments())
- );
+ partitions);
} finally {
busyLock.leaveBusy();
}
return completedFuture(null);
}
- /**
- * Listener of assignment configuration changes.
- *
- * @param assignmentsCtx Assignment configuration context.
- * @return A future.
- */
- private CompletableFuture<?> onUpdateAssignments(ConfigurationNotificationEvent<byte[]> assignmentsCtx) {
- if (!busyLock.enterBusy()) {
- return failedFuture(new NodeStoppingException());
- }
-
- try {
- return updateAssignmentInternal(assignmentsCtx)
- .whenComplete((v, e) -> {
- if (e == null) {
- for (var listener : assignmentsChangeListeners) {
- listener.accept(this);
- }
- }
- });
- } finally {
- busyLock.leaveBusy();
- }
- }
-
/**
* Updates or creates partition raft groups.
*
- * @param assignmentsCtx Change assignment event.
+ * @param evt Change assignment event.
Review Comment:
Fixed
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngine.java:
##########
@@ -187,39 +187,47 @@ public CompletableFuture<Void> onUpdate(WatchEvent evt) {
if (zoneId == tableZoneId) {
TableConfiguration tableCfg = tables.get(tableView.name());
- byte[] assignmentsBytes = ((ExtendedTableConfiguration) tableCfg).assignments().value();
+ int tableId = tableCfg.id().value();
- List<Set<Assignment>> tableAssignments = assignmentsBytes == null
- ? Collections.emptyList()
- : ByteUtils.fromBytes(assignmentsBytes);
+ CompletableFuture<List<Set<Assignment>>> tableAssignmentsFut = tableAssignments(
+ metaStorageManager,
+ tableId, distributionZoneConfiguration.partitions().value());
- for (int part = 0; part < distributionZoneConfiguration.partitions().value(); part++) {
- int tableId = tableCfg.id().value();
+ tableAssignmentsFut.thenAccept(tbls -> {
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sergeyuttsel commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1210247829
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##########
@@ -1038,7 +1038,9 @@ public void testOneNodeRestartWithGap() throws InterruptedException {
/**
* Checks that the table created in cluster of 2 nodes, is recovered on a node after restart of this node.
*/
- @Test
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-19506 here we receive the empty nodes from baseline manager,
+ // but should get the nodes from metastore instead
+ @Disabled("https://issues.apache.org/jira/browse/IGNITE-19506")
Review Comment:
Ok. But still this test can be fixed by using startNode instead of startPartialNode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1214215698
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/distribution/zones/ItDistributionZonesFilterTest.java:
##########
@@ -298,7 +295,16 @@ void testEmptyDataNodesDoNotPropagatedToStableAfterAlteringFilter() throws Excep
waitDataNodeAndListenersAreHandled(metaStorageManager, 2);
- assertPendingStableAreNull(metaStorageManager, partId);
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-19425 here we should have no nodes,
+ // which pass the filter, when dataNodes from DistributionZoneManager will be used
+ assertValueInStorage(
Review Comment:
Yes, you're right
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] kgusakov commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "kgusakov (via GitHub)" <gi...@apache.org>.
kgusakov commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1211725949
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/distribution/zones/ItDistributionZonesFilterTest.java:
##########
@@ -126,14 +124,6 @@ void testFilteredDataNodesPropagatedToStable() throws Exception {
TablePartitionId partId = new TablePartitionId(table.tableId(), 0);
- assertValueInStorage(
Review Comment:
Added the check with appropriate todo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1211650575
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/distribution/zones/ItDistributionZonesFilterTest.java:
##########
@@ -220,15 +199,15 @@ void testFilteredEmptyDataNodesDoNotTriggerRebalance() throws Exception {
TablePartitionId partId = new TablePartitionId(table.tableId(), 0);
// Table was created after both nodes was up, so there wasn't any rebalance.
- assertPendingStableAreNull(metaStorageManager, partId);
+ assertPendingsAreNull(metaStorageManager, partId);
Review Comment:
This method was needed to check that there wasn't any rebalance at all, now we check only pending, is this correct to check like this the absence of a rebalance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] vldpyatkov merged pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov merged PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sergeyuttsel commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1211626378
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java:
##########
@@ -1038,19 +1038,21 @@ public void testOneNodeRestartWithGap() throws InterruptedException {
/**
* Checks that the table created in cluster of 2 nodes, is recovered on a node after restart of this node.
*/
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-19506 here we receive the empty nodes from baseline manager,
Review Comment:
Seems that this todo is not needed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1211648514
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/distribution/zones/ItDistributionZonesFilterTest.java:
##########
@@ -126,14 +124,6 @@ void testFilteredDataNodesPropagatedToStable() throws Exception {
TablePartitionId partId = new TablePartitionId(table.tableId(), 0);
- assertValueInStorage(
Review Comment:
We somehow need to check that stable has been actually rewritten. Now we only check it at the end
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] kgusakov commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "kgusakov (via GitHub)" <gi...@apache.org>.
kgusakov commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1214058313
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/distribution/zones/ItDistributionZonesFilterTest.java:
##########
@@ -298,7 +295,16 @@ void testEmptyDataNodesDoNotPropagatedToStableAfterAlteringFilter() throws Excep
waitDataNodeAndListenersAreHandled(metaStorageManager, 2);
- assertPendingStableAreNull(metaStorageManager, partId);
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-19425 here we should have no nodes,
+ // which pass the filter, when dataNodes from DistributionZoneManager will be used
+ assertValueInStorage(
Review Comment:
The name and semantic of the test is testEmptyDataNodesDoNotPropagatedToStableAfterAlteringFilter, so, I thought it will be strange to check the pendings in the end
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] alievmirza commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1214047816
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/distribution/zones/ItDistributionZonesFilterTest.java:
##########
@@ -298,7 +295,16 @@ void testEmptyDataNodesDoNotPropagatedToStableAfterAlteringFilter() throws Excep
waitDataNodeAndListenersAreHandled(metaStorageManager, 2);
- assertPendingStableAreNull(metaStorageManager, partId);
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-19425 here we should have no nodes,
+ // which pass the filter, when dataNodes from DistributionZoneManager will be used
+ assertValueInStorage(
Review Comment:
why not `assertPendingWasNeverExsists`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sergeyuttsel commented on pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#issuecomment-1568302440
Previously it was discussed that usage of DistributionZoneManager#topologyVersionedDataNodes method will be removed in this pr. So why it is not removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] vldpyatkov commented on a diff in pull request #2070: IGNITE-19405 Remove assignments from table configuration
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #2070:
URL: https://github.com/apache/ignite-3/pull/2070#discussion_r1210783894
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngine.java:
##########
@@ -187,39 +187,47 @@ public CompletableFuture<Void> onUpdate(WatchEvent evt) {
if (zoneId == tableZoneId) {
TableConfiguration tableCfg = tables.get(tableView.name());
- byte[] assignmentsBytes = ((ExtendedTableConfiguration) tableCfg).assignments().value();
+ int tableId = tableCfg.id().value();
- List<Set<Assignment>> tableAssignments = assignmentsBytes == null
- ? Collections.emptyList()
- : ByteUtils.fromBytes(assignmentsBytes);
+ CompletableFuture<List<Set<Assignment>>> tableAssignmentsFut = tableAssignments(
+ metaStorageManager,
+ tableId, distributionZoneConfiguration.partitions().value());
- for (int part = 0; part < distributionZoneConfiguration.partitions().value(); part++) {
- int tableId = tableCfg.id().value();
+ tableAssignmentsFut.thenAccept(tbls -> {
Review Comment:
Why do you call assignments tbls?
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngine.java:
##########
@@ -187,39 +187,47 @@ public CompletableFuture<Void> onUpdate(WatchEvent evt) {
if (zoneId == tableZoneId) {
TableConfiguration tableCfg = tables.get(tableView.name());
- byte[] assignmentsBytes = ((ExtendedTableConfiguration) tableCfg).assignments().value();
+ int tableId = tableCfg.id().value();
- List<Set<Assignment>> tableAssignments = assignmentsBytes == null
- ? Collections.emptyList()
- : ByteUtils.fromBytes(assignmentsBytes);
+ CompletableFuture<List<Set<Assignment>>> tableAssignmentsFut = tableAssignments(
+ metaStorageManager,
+ tableId, distributionZoneConfiguration.partitions().value());
- for (int part = 0; part < distributionZoneConfiguration.partitions().value(); part++) {
- int tableId = tableCfg.id().value();
+ tableAssignmentsFut.thenAccept(tbls -> {
- TablePartitionId replicaGrpId = new TablePartitionId(tableId, part);
+ for (int part = 0; part < distributionZoneConfiguration.partitions().value(); part++) {
- int replicas = distributionZoneConfiguration.replicas().value();
+ TablePartitionId replicaGrpId = new TablePartitionId(tableId, part);
- int partId = part;
+ int replicas = distributionZoneConfiguration.replicas().value();
- updatePendingAssignmentsKeys(
- tableView.name(),
- replicaGrpId,
- filteredDataNodes,
- replicas,
- evt.entryEvent().newEntry().revision(),
- metaStorageManager,
- part,
- tableAssignments.isEmpty() ? Collections.emptySet() : tableAssignments.get(part)
- ).exceptionally(e -> {
- LOG.error(
- "Exception on updating assignments for [table={}, partition={}]", e, tableView.name(),
- partId
- );
-
- return null;
- });
- }
+ int partId = part;
+
+ updatePendingAssignmentsKeys(
+ tableView.name(),
+ replicaGrpId,
+ filteredDataNodes,
+ replicas,
+ evt.entryEvent().newEntry().revision(),
+ metaStorageManager,
+ partId,
+ tbls.isEmpty() ? Collections.emptySet() : tbls.get(partId)
Review Comment:
This is a suspicious condition.
What happens if one assignment is not created in MS?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -574,63 +623,38 @@ private CompletableFuture<?> onTableDelete(ConfigurationNotificationEvent<TableV
}
try {
+ int partitions =
+ getZoneById(distributionZonesConfiguration, ctx.oldValue().zoneId()).value().partitions();
+
dropTableLocally(
ctx.storageRevision(),
ctx.oldValue().name(),
ctx.oldValue().id(),
- ByteUtils.fromBytes(ctx.oldValue(ExtendedTableView.class).assignments())
- );
+ partitions);
} finally {
busyLock.leaveBusy();
}
return completedFuture(null);
}
- /**
- * Listener of assignment configuration changes.
- *
- * @param assignmentsCtx Assignment configuration context.
- * @return A future.
- */
- private CompletableFuture<?> onUpdateAssignments(ConfigurationNotificationEvent<byte[]> assignmentsCtx) {
- if (!busyLock.enterBusy()) {
- return failedFuture(new NodeStoppingException());
- }
-
- try {
- return updateAssignmentInternal(assignmentsCtx)
- .whenComplete((v, e) -> {
- if (e == null) {
- for (var listener : assignmentsChangeListeners) {
- listener.accept(this);
- }
- }
- });
- } finally {
- busyLock.leaveBusy();
- }
- }
-
/**
* Updates or creates partition raft groups.
*
- * @param assignmentsCtx Change assignment event.
+ * @param evt Change assignment event.
Review Comment:
Please, describe all method parameters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org