hemantk-12 commented on code in PR #4249:
URL: https://github.com/apache/ozone/pull/4249#discussion_r1099045421
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java:
##########
@@ -440,10 +440,10 @@ private String
addToBatch(Queue<DoubleBufferEntry<OMClientResponse>> buffer,
// 1. It is first element in the response,
// 2. Current request is createSnapshot request.
// 3. Previous request was createSnapshot request.
- if (response.isEmpty() ||
- omResponse.getCreateSnapshotResponse() != null ||
- (previousOmResponse != null &&
- previousOmResponse.getCreateSnapshotResponse() != null)) {
+ if (response.isEmpty() || omResponse.getCreateSnapshotResponse()
Review Comment:
I believe that warning can be ignored in this case. Even "Java protocol
buffer methods accept or return nulls", wouldn't having a null check would be
more appropriate? Because when it is not set or set to null, will surely return
null.
May be something like `(omResponse.getCreateSnapshotResponse() != null &&
omResponse.getCreateSnapshotResponse().hasSnapshotInfo())`. Tho I think just
null check should work.
I ran
[test](https://github.com/apache/ozone/pull/4249/files#diff-195575b08642fad83fad37b0cbe2ba5fa39f9db550028ac540523de01ecff3a9R56)
in IntelliJ with your changes and it failed. Can you please double check, run
it locally on your machine and verify that it passes?
```
2023-02-07 10:40:46,892 [main] ERROR ratis.OzoneManagerDoubleBuffer
(ExitUtils.java:terminate(133)) - Terminating with exit status 2: During flush
to DB encountered error in OMDoubleBuffer flush thread main
java.lang.NullPointerException
at
org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.splitReadyBufferAtCreateSnapshot(OzoneManagerDoubleBuffer.java:444)
at
org.apache.hadoop.ozone.om.ratis.OzoneManagerDoubleBuffer.flushCurrentBuffer(OzoneManagerDoubleBuffer.java:291)
at
org.apache.hadoop.ozone.om.ratis.TestOzoneManagerDoubleBuffer.testOzoneManagerDoubleBuffer(TestOzoneManagerDoubleBuffer.java:198)
```
Also I don't see if this test was run as part of unit test workflow:
https://github.com/apache/ozone/actions/runs/4114612552/jobs/7102217772 .
Please correct me if it is supposed to run in some other workflow.
One more point if `omResponse.getCreateSnapshotResponse() == null` is always
true, test case-1 should fail where double buffer has
`Arrays.asList(omKeyCreateResponse, omBucketCreateResponse)`. It should get
split into two but it doesn't not.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -302,14 +303,19 @@ protected OmMetadataManagerImpl() {
}
// metadata constructor for snapshots
- private OmMetadataManagerImpl(OzoneConfiguration conf, String
snapshotDirName)
- throws IOException {
+ private OmMetadataManagerImpl(OzoneConfiguration conf, String
snapshotDirName,
+ boolean isSnapshotInCache) throws IOException {
lock = new OmReadOnlyLock();
omEpoch = 0;
String snapshotDir = OMStorage.getOmDbDir(conf) +
OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
- setStore(loadDB(conf, new File(snapshotDir),
- OM_DB_NAME + snapshotDirName, true));
+ File metaDir = new File(snapshotDir);
+ String dbName = OM_DB_NAME + snapshotDirName;
+ if (isSnapshotInCache) {
Review Comment:
It would be great if add this as a comment in code. Also mentioned "It is
most likely DB entries will get flushed in this wait time.".
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -302,14 +303,19 @@ protected OmMetadataManagerImpl() {
}
// metadata constructor for snapshots
- private OmMetadataManagerImpl(OzoneConfiguration conf, String
snapshotDirName)
- throws IOException {
+ private OmMetadataManagerImpl(OzoneConfiguration conf, String
snapshotDirName,
+ boolean isSnapshotInCache) throws IOException {
lock = new OmReadOnlyLock();
omEpoch = 0;
String snapshotDir = OMStorage.getOmDbDir(conf) +
OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
- setStore(loadDB(conf, new File(snapshotDir),
- OM_DB_NAME + snapshotDirName, true));
+ File metaDir = new File(snapshotDir);
+ String dbName = OM_DB_NAME + snapshotDirName;
+ if (isSnapshotInCache) {
+ File checkpoint = Paths.get(metaDir.toPath().toString(),
dbName).toFile();
+ RDBCheckpointManager.waitForCheckpointDirectoryExist(checkpoint);
Review Comment:
No sure if "checkpoint in
OmCreateSnapshotCreateRequest#validateAndUpdateCache inside the bucket lock"
would work since dir creation is also an async RocksDB operation. But it is
worth a try.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
