You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "Cyrill (via GitHub)" <gi...@apache.org> on 2023/09/07 13:56:03 UTC
[GitHub] [ignite-3] Cyrill opened a new pull request, #2562: IGNITE-20379 Concurrency issues with write intent tracking
Cyrill opened a new pull request, #2562:
URL: https://github.com/apache/ignite-3/pull/2562
https://issues.apache.org/jira/browse/IGNITE-20379
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
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1318701349
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PendingRows.java:
##########
@@ -81,8 +77,12 @@ public void addPendingRowIds(UUID txId, Collection<RowId> rowIds) {
*
* @param txId Transaction ID.
*/
- public void removePendingRowIds(UUID txId) {
- txsPendingRowIds.remove(txId);
+ public void removePendingRowIds(UUID txId, Set<RowId> pendingRowIds) {
Review Comment:
It's not a problem. First of all generally speaking lazy writeIntent resolution gives us correctness. Cleanup on tx finish is an optimization, very important one, but still an optimization. We don't expect such crashes to be an often case, so that we might prefer pimplier API to rare performance related inefficiency.
--
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] sanpwc commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1318681649
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/RaftGroupEventsClientListener.java:
##########
@@ -79,15 +79,19 @@ public void removeLeaderElectionListener(ReplicationGroupId groupId, LeaderElect
* @param term Election term.
*/
public void onLeaderElected(ReplicationGroupId groupId, ClusterNode leader, long term) {
Review Comment:
I'd rather add copy on write list as a value because it's not on the hot path.
--
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] sanpwc commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1318701349
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PendingRows.java:
##########
@@ -81,8 +77,12 @@ public void addPendingRowIds(UUID txId, Collection<RowId> rowIds) {
*
* @param txId Transaction ID.
*/
- public void removePendingRowIds(UUID txId) {
- txsPendingRowIds.remove(txId);
+ public void removePendingRowIds(UUID txId, Set<RowId> pendingRowIds) {
Review Comment:
It's not a problem. First of all generally speaking lazy writeIntent resolution gives us correctness. Cleanup on tx finish is an optimization, very important one, but still optimization. We don't expect such crashes to be an often case, so that we might prefer pimplier API to rare performance related inefficiency.
--
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] Cyrill commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1318689347
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PendingRows.java:
##########
@@ -81,8 +77,12 @@ public void addPendingRowIds(UUID txId, Collection<RowId> rowIds) {
*
* @param txId Transaction ID.
*/
- public void removePendingRowIds(UUID txId) {
- txsPendingRowIds.remove(txId);
+ public void removePendingRowIds(UUID txId, Set<RowId> pendingRowIds) {
Review Comment:
Imagine we get the rows, start iterating over them and crash. A subsequent retry will not see these rows if we just remove them.
--
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] Cyrill commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1320523115
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PendingRows.java:
##########
@@ -81,8 +77,12 @@ public void addPendingRowIds(UUID txId, Collection<RowId> rowIds) {
*
* @param txId Transaction ID.
*/
- public void removePendingRowIds(UUID txId) {
- txsPendingRowIds.remove(txId);
+ public void removePendingRowIds(UUID txId, Set<RowId> pendingRowIds) {
Review Comment:
I wonder whether the cleanup on finish is only the optimization.
Please correct me I'm wrong, but imagine the case when there is a committed transaction and the storage still contains a write intent due to a missing cleanup.
If we are doing the resolution for a RO transaction, we are good. We return the value of this write intent.
If we are doing the same for a RW transaction, the resolution will return the row, which is still a write intent from the storage point of view, but any following insert with the same row id will throw an exception from the storage, which does not allow more than one WI for a row.
Is there something I'm missing which makes my assumptions wrong?
--
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] Cyrill commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1319569853
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/RaftGroupEventsClientListener.java:
##########
@@ -79,15 +79,19 @@ public void removeLeaderElectionListener(ReplicationGroupId groupId, LeaderElect
* @param term Election term.
*/
public void onLeaderElected(ReplicationGroupId groupId, ClusterNode leader, long term) {
Review Comment:
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
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1320554891
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PendingRows.java:
##########
@@ -81,8 +77,12 @@ public void addPendingRowIds(UUID txId, Collection<RowId> rowIds) {
*
* @param txId Transaction ID.
*/
- public void removePendingRowIds(UUID txId) {
- txsPendingRowIds.remove(txId);
+ public void removePendingRowIds(UUID txId, Set<RowId> pendingRowIds) {
Review Comment:
If RW transaction touches a writeIntent it'll do the cleanup before any further activities. It's a mandatory action, however it's not a cleanup on tx finish that I've mentioned previously, meaning that tx finish cleanup still should be considered as an optimization. It's absolutely required for a RW transaction to cleanup specific writeIntent that was touched, however in addition to that, as we previously discussed, it may try to cleanup all local writeIntents with same txId.
All in all, I still believe that removeAndGet should work here.
--
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] Cyrill commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1320523115
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PendingRows.java:
##########
@@ -81,8 +77,12 @@ public void addPendingRowIds(UUID txId, Collection<RowId> rowIds) {
*
* @param txId Transaction ID.
*/
- public void removePendingRowIds(UUID txId) {
- txsPendingRowIds.remove(txId);
+ public void removePendingRowIds(UUID txId, Set<RowId> pendingRowIds) {
Review Comment:
I wonder whether cleanup on finish is actually the optimization.
Please correct me I'm wrong, but if there is a committed transaction, but the storage still contains a write intent due to a missing cleanup.
If we are doing the resolution for a RO transaction, we are good.
If we are doing the same for a RW transaction, the resolution will return the row, which is still a write intent from the storage point of view. And any following insert with the same row id will throw an exception.
--
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] Cyrill commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1320523115
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PendingRows.java:
##########
@@ -81,8 +77,12 @@ public void addPendingRowIds(UUID txId, Collection<RowId> rowIds) {
*
* @param txId Transaction ID.
*/
- public void removePendingRowIds(UUID txId) {
- txsPendingRowIds.remove(txId);
+ public void removePendingRowIds(UUID txId, Set<RowId> pendingRowIds) {
Review Comment:
I wonder whether the cleanup on finish is only the optimization.
Please correct me I'm wrong, but if there is a committed transaction, but the storage still contains a write intent due to a missing cleanup.
If we are doing the resolution for a RO transaction, we are good.
If we are doing the same for a RW transaction, the resolution will return the row, which is still a write intent from the storage point of view. And any following insert with the same row id will throw an exception.
--
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] sanpwc commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1318669069
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PendingRows.java:
##########
@@ -81,8 +77,12 @@ public void addPendingRowIds(UUID txId, Collection<RowId> rowIds) {
*
* @param txId Transaction ID.
*/
- public void removePendingRowIds(UUID txId) {
- txsPendingRowIds.remove(txId);
+ public void removePendingRowIds(UUID txId, Set<RowId> pendingRowIds) {
Review Comment:
It is complicated. Are there any use-cases where we need to retrieve rowIds for further iterations without removal?
If not I'd rather add removeAndGet() of similar.
--
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] Cyrill commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1320523115
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PendingRows.java:
##########
@@ -81,8 +77,12 @@ public void addPendingRowIds(UUID txId, Collection<RowId> rowIds) {
*
* @param txId Transaction ID.
*/
- public void removePendingRowIds(UUID txId) {
- txsPendingRowIds.remove(txId);
+ public void removePendingRowIds(UUID txId, Set<RowId> pendingRowIds) {
Review Comment:
I wonder whether the cleanup on finish is only the optimization.
Please correct me I'm wrong, but imagine the case when there is a committed transaction and the storage still contains a write intent due to a missing cleanup.
If we are doing the resolution for a RO transaction, we are good. We return the value of this write intent.
If we are doing the same for a RW transaction, the resolution will return the row, which is still a write intent from the storage point of view, but any following insert with the same row id will throw an exception from the storage, which does not allow more than one WI for a row.
So we need to wait for a row cleanup on RW resolution to fix this. If we do that, this change will be less of a priority and can be replaced with getAndRemove.
Is there something I'm missing which makes my assumptions wrong?
--
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] Cyrill commented on a diff in pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562#discussion_r1320523115
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PendingRows.java:
##########
@@ -81,8 +77,12 @@ public void addPendingRowIds(UUID txId, Collection<RowId> rowIds) {
*
* @param txId Transaction ID.
*/
- public void removePendingRowIds(UUID txId) {
- txsPendingRowIds.remove(txId);
+ public void removePendingRowIds(UUID txId, Set<RowId> pendingRowIds) {
Review Comment:
I wonder whether the cleanup on finish is only the optimization.
Please correct me I'm wrong, but imagine the case when there is a committed transaction and the storage still contains a write intent due to a missing cleanup.
If we are doing the resolution for a RO transaction, we are good. We return the value of this write intent.
If we are doing the same for a RW transaction, the resolution will return the row, which is still a write intent from the storage point of view, but any following insert with the same row id will throw an exception from the storage, which does not allow more than one WI for a row
--
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] sanpwc merged pull request #2562: IGNITE-20379 Concurrency issues with write intent tracking
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc merged PR #2562:
URL: https://github.com/apache/ignite-3/pull/2562
--
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