You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "JAkutenshi (via GitHub)" <gi...@apache.org> on 2024/04/08 20:48:37 UTC
[PR] IGNITE-22001 Throw specific exception if during writeTableAssignmentsToMetastore process was interrupted [ignite-3]
JAkutenshi opened a new pull request, #3575:
URL: https://github.com/apache/ignite-3/pull/3575
The problem was in returning null in cases of `TableManger#writeTableAssignmentsToMetastore` interruption while internal future proceeds. It leads to not caught issues after the method callings.
The solution is to throw a specific exception to detect and easily fix any problem during attempt to write assignments into a metastore.
In the result:
- `WritingAssignmentsToMetastoreException` class is implemented.
- In `.exceptionally(...)` case of `TableManger#writeTableAssignmentsToMetastore` now the new exception is thrown.
- `TableManagerTest#testWriteTableAssignmentsToMetastoreExceptionally` is introduced with 2 main cases of the method interruption: after and before metastore's invocation completion.
There is an issue that in `.exceptionally(...)` for main assignments future we can't got access to completed list, then exception class has additional `isAssignmentsWereAvailable()` method to check if the list wasn't available in a scope while exception throwing. And in `org/apache/ignite/internal/table/distributed/TableManager.java:755` made a check for the nested exception duplicity evasion.
---
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- 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 description of the JIRA ticket explains WHAT was made, WHY and 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.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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
Re: [PR] IGNITE-22001 Throw specific exception if during writeTableAssignmentsToMetastore process was interrupted [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3575:
URL: https://github.com/apache/ignite-3/pull/3575#discussion_r1570539678
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -738,24 +755,31 @@ private CompletableFuture<List<Assignments>> writeTableAssignmentsToMetastore(
Entry assignmentsEntry = metaStorageAssignments.get(stablePartAssignmentsKey(partId));
assert assignmentsEntry != null && !assignmentsEntry.empty() && !assignmentsEntry.tombstone()
- : "Unexpected assignments for partition [" + partId + ", entry=" + assignmentsEntry + "].";
Review Comment:
Why?
--
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
Re: [PR] IGNITE-22001 Throw specific exception if during writeTableAssignmentsToMetastore process was interrupted [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc merged PR #3575:
URL: https://github.com/apache/ignite-3/pull/3575
--
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
Re: [PR] IGNITE-22001 Throw specific exception if during writeTableAssignmentsToMetastore process was interrupted [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3575:
URL: https://github.com/apache/ignite-3/pull/3575#discussion_r1570529696
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -135,4 +136,14 @@ public static Assignments fromBytes(byte @Nullable [] bytes) {
public String toString() {
return S.toString(this);
}
+
+ /**
+ * Creates a string representation of the given assignments list for logging usage purpose mostly.
+ *
+ * @param assignments List of assignments to present as string.
+ * @return String representation of the given assignments list.
+ */
+ public static String assignmentListToString(List<Assignments> assignments) {
Review Comment:
Why we do not use toString for that purpose?
--
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
Re: [PR] IGNITE-22001 Throw specific exception if during writeTableAssignmentsToMetastore process was interrupted [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on PR #3575:
URL: https://github.com/apache/ignite-3/pull/3575#issuecomment-2058381339
Fix done!
--
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
Re: [PR] IGNITE-22001 Throw specific exception if during writeTableAssignmentsToMetastore process was interrupted [ignite-3]
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #3575:
URL: https://github.com/apache/ignite-3/pull/3575#discussion_r1568686202
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -752,10 +762,14 @@ private CompletableFuture<List<Assignments>> writeTableAssignmentsToMetastore(
});
}
})
- .exceptionally(e -> {
- LOG.error("Couldn't write assignments to metastore", e);
-
- return null;
+ .handle((realAssignments, e) -> {
+ if (e != null) {
+ LOG.error("Couldn't write assignments {} to metastore during invoke",
Review Comment:
from my point of view, the message is a bit incorrect, exception could caught here not only from invoke, let's rephrase 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
Re: [PR] IGNITE-22001 Throw specific exception if during writeTableAssignmentsToMetastore process was interrupted [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3575:
URL: https://github.com/apache/ignite-3/pull/3575#discussion_r1570538748
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -707,20 +707,37 @@ private CompletableFuture<List<Assignments>> writeTableAssignmentsToMetastore(
List<Operation> partitionAssignments = new ArrayList<>(newAssignments.size());
for (int i = 0; i < newAssignments.size(); i++) {
- partitionAssignments.add(put(
- stablePartAssignmentsKey(
- new TablePartitionId(tableId, i)),
- newAssignments.get(i).toBytes()));
+ ByteArray stableAssignmentsKey = stablePartAssignmentsKey(new TablePartitionId(tableId, i));
+ byte[] anAssignment = newAssignments.get(i).toBytes();
+ Operation op = put(stableAssignmentsKey, anAssignment);
+ partitionAssignments.add(op);
}
Condition condition = notExists(new ByteArray(partitionAssignments.get(0).key()));
return metaStorageMgr
.invoke(condition, partitionAssignments, Collections.emptyList())
+ .handle((invokeResult, e) -> {
+ if (e != null) {
+ LOG.error(
+ "Couldn't write assignments {} to metastore during invoke",
Review Comment:
Should we also wrap assignments with square braces?
--
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
Re: [PR] IGNITE-22001 Throw specific exception if during writeTableAssignmentsToMetastore process was interrupted [ignite-3]
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on PR #3575:
URL: https://github.com/apache/ignite-3/pull/3575#issuecomment-2057432221
Let's try to do the task without introducing a new exception. With the new proposed exception, we have to throw it every time we change assignments in the metastore. Let's try to not increase a lot the scope of the task.
--
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
Re: [PR] IGNITE-22001 Throw specific exception if during writeTableAssignmentsToMetastore process was interrupted [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3575:
URL: https://github.com/apache/ignite-3/pull/3575#discussion_r1570567340
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1240,32 +1264,16 @@ private CompletableFuture<?> createTableLocally(
) {
return inBusyLockAsync(busyLock, () -> {
int tableId = tableDescriptor.id();
- int zoneId = tableDescriptor.zoneId();
// Retrieve descriptor during synchronous call, before the previous catalog version could be concurrently compacted.
CatalogZoneDescriptor zoneDescriptor = getZoneDescriptor(tableDescriptor, catalogVersion);
- CompletableFuture<List<Assignments>> assignmentsFuture;
-
- // Check if the table already has assignments in the meta storage locally.
- // So, it means, that it is a recovery process and we should use the meta storage local assignments instead of calculation
- // of the new ones.
- if (partitionAssignmentsGetLocally(metaStorageMgr, tableId, 0, causalityToken) != null) {
- assignmentsFuture = completedFuture(
- tableAssignmentsGetLocally(metaStorageMgr, tableId, zoneDescriptor.partitions(), causalityToken));
- } else {
- assignmentsFuture = distributionZoneManager.dataNodes(causalityToken, catalogVersion, zoneId)
- .thenApply(dataNodes -> AffinityUtils.calculateAssignments(
- dataNodes,
- zoneDescriptor.partitions(),
- zoneDescriptor.replicas()
- ).stream().map(Assignments::of).collect(toList()));
-
- assignmentsFuture.thenAccept(assignmentsList -> {
- LOG.info(IgniteStringFormatter.format("Assignments calculated from data nodes [table={}, tableId={}, assignments={}, "
- + "revision={}]", tableDescriptor.name(), tableId, assignmentListToString(assignmentsList), causalityToken));
- });
- }
+ CompletableFuture<List<Assignments>> assignmentsFuture = createOrGetAssignments(
Review Comment:
getOrCreate
--
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
Re: [PR] IGNITE-22001 Throw specific exception if during writeTableAssignmentsToMetastore process was interrupted [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3575:
URL: https://github.com/apache/ignite-3/pull/3575#discussion_r1570529696
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -135,4 +136,14 @@ public static Assignments fromBytes(byte @Nullable [] bytes) {
public String toString() {
return S.toString(this);
}
+
+ /**
+ * Creates a string representation of the given assignments list for logging usage purpose mostly.
+ *
+ * @param assignments List of assignments to present as string.
+ * @return String representation of the given assignments list.
+ */
+ public static String assignmentListToString(List<Assignments> assignments) {
Review Comment:
Why we do not use toString for that purpose?
--
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