-
Notifications
You must be signed in to change notification settings - Fork 414
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.
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
RATIS-1855 Fix some sonar code smell and bugs in ratis-server #892
Conversation
Signed-off-by: OneSizeFitQuorum <[email protected]>
PTAL @szetszwo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OneSizeFitsQuorum , thanks a lot for working on this! Please see the comments inlined.
@@ -39,7 +40,7 @@ public class RaftStorageImpl implements RaftStorage { | |||
private final StartupOption startupOption; | |||
private final CorruptionPolicy logCorruptionPolicy; | |||
private volatile StorageState state = StorageState.UNINITIALIZED; | |||
private volatile RaftStorageMetadataFileImpl metaFile; | |||
private final AtomicReference<RaftStorageMetadataFileImpl> metaFile = new AtomicReference<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create a class:
static class MetaFile {
private final AtomicReference<RaftStorageMetadataFileImpl> ref = new AtomicReference<>();
RaftStorageMetadataFile get() {
return ref.get();
}
RaftStorageMetadataFile set(File file) {
final RaftStorageMetadataFileImpl impl = new RaftStorageMetadataFileImpl(file);
ref.set(impl);
return impl;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good advice~
metaFile.set(new RaftStorageMetadataFileImpl(storageDir.getMetaFile())); | ||
metaFile.get().persist(RaftStorageMetadata.getDefault()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After created a class, this becomes a single llne
metaFile.set(storageDir.getMetaFile()).persist(RaftStorageMetadata.getDefault());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
metaFile.set(new RaftStorageMetadataFileImpl(f)); | ||
final RaftStorageMetadata metadata = metaFile.get().getMetadata(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, this becomes a single llne
final RaftStorageMetadata metadata = metaFile.set(f).getMetadata();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
@@ -150,7 +152,7 @@ private RaftStorageImpl format() throws IOException { | |||
} | |||
throw new IOException("Failed to FORMAT a new storage dir for " + storageDirName + " from " + dirsInConf); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep this empty line.
@@ -130,13 +131,14 @@ RaftStorageImpl run() throws IOException { | |||
} | |||
} | |||
|
|||
@SuppressWarnings("java:S1181") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment at the end for this and the other SuppressWarnings?
@SuppressWarnings("java:S1181") // catch Throwable
@SuppressWarnings("java:S2095") // return Closable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
Signed-off-by: OneSizeFitQuorum <[email protected]>
c3b779c
to
a39249f
Compare
There was a problem hiding this 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.
see jira1855