Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RATIS-2158. Let the snapshot sender and receiver use a new digester each time #1151

Merged
merged 7 commits into from
Sep 19, 2024
Merged

RATIS-2158. Let the snapshot sender and receiver use a new digester each time #1151

merged 7 commits into from
Sep 19, 2024

Conversation

133tosakarin
Copy link
Contributor

@OneSizeFitsQuorum
Copy link
Contributor

@szetszwo PTAL

szetszwo
szetszwo reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@133tosakarin , @OneSizeFitsQuorum , on a second thought, we may not need any synchronization. The code must be already synchronized. If not, two different threads accessing digester can change order. Then the final digester.digest() will be different.

The reason is similar to OutputStream, whose subclasses (e.g. FileOutputStream) usually are NOT thread-safe. If there is a need, the caller should synchronize it; see https://stackoverflow.com/questions/8422278/write-to-fileoutputstream-from-multiple-threads-in-java

If we really want to make a change, how about simply add volatile?

ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java Outdated
@@ -87,7 +89,7 @@ private FileChannel open(FileChunkProto chunk, File tmpSnapshotFile) throws IOEx
}
// create the temp snapshot file and put padding inside
out = FileUtils.newFileChannel(tmpSnapshotFile, StandardOpenOption.WRITE, StandardOpenOption.CREATE);
digester = MD5Hash.newDigester();
digester.set(MD5Hash.getDigester());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@133tosakarin , why changing back to getDigester?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I mistakenly thought that every time I use DIGESTER_FACTORY (), newDigest () will be called, so I want to try changing it to getDigester ().

I have changed back to newDigester.

ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java Outdated
@@ -62,7 +63,7 @@ public class SnapshotManager {
private final Supplier<File> snapshotDir;
private final Supplier<File> snapshotTmpDir;
private final Function<FileChunkProto, String> getRelativePath;
private MessageDigest digester;
private final AtomicReference<MessageDigest> digester;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about simply add a volatile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

szetszwo
szetszwo reviewed Sep 16, 2024
View reviewed changes
ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java Outdated
@@ -62,7 +63,7 @@ public class SnapshotManager {
private final Supplier<File> snapshotDir;
private final Supplier<File> snapshotTmpDir;
private final Function<FileChunkProto, String> getRelativePath;
private MessageDigest digester;
private volatile AtomicReference<MessageDigest> digester;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@133tosakarin , sorry, I mean that don't use AtomicReference, i.e.

 private volatile MessageDigest digester;

szetszwo
szetszwo approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 the change looks good.

Let's also update the title?

@OneSizeFitsQuorum
Copy link
Contributor

Hi, @szetszwo @133tosakarin ,

  1. Maybe we need to create a new one for sender as we already saw that the reset on the receiver side can sometimes fail, and there is no significant performance cost to creating a new digester?
  2. Thanks for the reminder. Now that we have synchronized, we probably don't need to add another judgment; adding volatile would cause a sonar bug. Maybe we can undo this change.
  3. It seems the ci failure is same as this problem discussed in mail list, then we can merge this pr first and create a new issue for this problem for master.
image

@szetszwo
Copy link
Contributor

t seems the ci failure is same as this problem discussed in mail list, ...

@OneSizeFitsQuorum , if the failure is related to ReferenceCount or zero-copy, let ignore it for the moment. We will fix it separately.

@133tosakarin 133tosakarin changed the title RATIS-2158. Wrap MessageDigest with AtomicReference for concurrency safety in SnapshotManager RATIS-2158. Add volatile key word to digester in SnapshotManager Sep 18, 2024
@133tosakarin 133tosakarin changed the title RATIS-2158. Add volatile key word to digester in SnapshotManager RATIS-2158. Let the snapshot sender and receiver use a new digester each time Sep 19, 2024
szetszwo
szetszwo reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@133tosakarin , thanks for the update! Please see the comment inlined.

ratis-common/src/main/java/org/apache/ratis/io/MD5Hash.java Outdated
@@ -34,7 +34,9 @@ public class MD5Hash {

public static MessageDigest newDigester() {
try {
return MessageDigest.getInstance("MD5");
MessageDigest digester = MessageDigest.getInstance("MD5");
digester.reset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the API, MessageDigest.getInstance(..) returns a new object. So, reset() is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all right

OneSizeFitsQuorum
OneSizeFitsQuorum approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

szetszwo
szetszwo approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 the change looks good.

@OneSizeFitsQuorum OneSizeFitsQuorum merged commit 8a50099 into apache:master Sep 19, 2024
11 of 12 checks passed
OneSizeFitsQuorum pushed a commit that referenced this pull request Sep 19, 2024
@133tosakarin @OneSizeFitsQuorum
RATIS-2158. Let the snapshot sender and receiver use a new digester e…
d7709da 
…ach time (#1151)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szetszwo szetszwo szetszwo approved these changes

@OneSizeFitsQuorum OneSizeFitsQuorum OneSizeFitsQuorum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@133tosakarin @OneSizeFitsQuorum @szetszwo