sashapolo commented on code in PR #7275:
URL: https://github.com/apache/ignite-3/pull/7275#discussion_r2638962752
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AddWriteLinkingWiInvokeClosure.java:
##########
@@ -108,27 +114,18 @@ protected RowVersion insertAnotherRowVersion(VersionChain
oldRow, @Nullable RowV
return newVersion;
}
- private static WriteIntentLinks newWiLinksForReplacement(
- @Nullable RowVersion existingWriteIntent,
- boolean replacingExistingWriteIntent,
- long wiListHeadLink
- ) {
- assert replacingExistingWriteIntent == (existingWriteIntent != null);
+ private WriteIntentLinks newWiLinksForReplacement(@Nullable RowVersion
existingWriteIntent, long wiListHeadLink) {
+ assert persistentStorage.writeIntentHeadIsLockedByCurrentThread();
- long newNextWiLink;
- long newPrevWiLink;
- if (replacingExistingWriteIntent) {
- newNextWiLink =
existingWriteIntent.operations().nextWriteIntentLink(wiListHeadLink);
- newPrevWiLink =
existingWriteIntent.operations().prevWriteIntentLink();
- } else {
- newNextWiLink = wiListHeadLink;
- newPrevWiLink = NULL_LINK;
+ if (existingWriteIntent instanceof WiLinkableRowVersion) {
+ // Re-read the links under WI list lock to be sure they are
up-to-date (this is a form of the double-checked lccking idiom).
Review Comment:
```suggestion
// Re-read the links under WI list lock to be sure they are
up-to-date (this is a form of the double-checked locking idiom).
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AddWriteLinkingWiInvokeClosure.java:
##########
@@ -83,22 +83,28 @@ protected RowVersion insertFirstRowVersion() {
}
@Override
- protected RowVersion insertAnotherRowVersion(VersionChain oldRow,
@Nullable RowVersion existingWriteIntent) {
- boolean replacingExistingWriteIntent = oldRow.isUncommitted();
+ protected RowVersion insertAnotherRowVersion(VersionChain chain, @Nullable
RowVersion existingWriteIntent) {
+ boolean replacingExistingWriteIntent = chain.isUncommitted();
assert replacingExistingWriteIntent == (existingWriteIntent != null);
- WiLinkableRowVersion newVersion =
insertRowVersion(oldRow.newestCommittedLink());
+ WiLinkableRowVersion newVersion =
insertWiLinkableRowVersion(chain.newestCommittedLink());
long wiListHeadLink = persistentStorage.lockWriteIntentListHead();
long newWiListHeadLink = wiListHeadLink;
try {
- WriteIntentLinks newWiLinks =
newWiLinksForReplacement(existingWriteIntent, replacingExistingWriteIntent,
wiListHeadLink);
+ WriteIntentLinks newWiLinks =
newWiLinksForReplacement(existingWriteIntent, wiListHeadLink);
updateWiListLinks(newVersion.link(), newWiLinks);
- if (!replacingExistingWriteIntent) {
- // Add our new version to the head of the WI list.
+ // We do not zero out links in the replaced write intent as no one
will be able to use them to traverse WI list
Review Comment:
Why do we need to zero out the links in the first place?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/WriteIntentListSupport.java:
##########
@@ -35,6 +35,7 @@ static void removeNodeFromWriteIntentsList(
long wiListHeadLink = storage.lockWriteIntentListHead();
try {
+ // Re-read the links under WI list lock to be sure they are
up-to-date (this is a form of the double-checked lccking idiom).
Review Comment:
```suggestion
// Re-read the links under WI list lock to be sure they are
up-to-date (this is a form of the double-checked locking idiom).
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
