adoroszlai commented on code in PR #4340:
URL: https://github.com/apache/ozone/pull/4340#discussion_r1140323431
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java:
##########
@@ -369,6 +369,23 @@ public void testDbStoreClosedOnBadVolumeWithDbVolumes()
throws IOException {
assertEquals(0, DatanodeStoreCache.getInstance().size());
}
+ @Test
+ public void testFailedVolumeSpace() throws IOException {
+
+ // Build failed volume
+ HddsVolume volume = volumeBuilder.failedVolume(true).build();
+
+ // In case of failed volume all stats should return 0.
+ assertEquals(0, volume.getVolumeInfoStats().getUsed());
+ assertEquals(0, volume.getVolumeInfoStats().getAvailable());
+ assertEquals(0, volume.getVolumeInfoStats().getCapacity());
+ assertEquals(0, volume.getVolumeInfoStats().getReserved());
+ assertEquals(0, volume.getVolumeInfoStats().getTotalCapacity());
+
+ // Shutdown the volume.
+ volume.shutdown();
Review Comment:
Cleanup should be performed in `finally` block.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/StorageVolume.java:
##########
@@ -115,18 +116,19 @@ protected StorageVolume(Builder<?> b) throws IOException {
if (!b.failedVolume) {
StorageLocation location = StorageLocation.parse(b.volumeRootStr);
storageDir = new File(location.getUri().getPath(), b.storageDirStr);
- this.volumeInfo = new VolumeInfo.Builder(b.volumeRootStr, b.conf)
+ this.volumeInfo = Optional.of(
+ new VolumeInfo.Builder(b.volumeRootStr, b.conf)
.storageType(b.storageType)
.usageCheckFactory(b.usageCheckFactory)
- .build();
+ .build());
this.volumeSet = b.volumeSet;
this.state = VolumeState.NOT_INITIALIZED;
this.clusterID = b.clusterID;
this.datanodeUuid = b.datanodeUuid;
this.conf = b.conf;
} else {
storageDir = new File(b.volumeRootStr);
- this.volumeInfo = null;
+ this.volumeInfo = Optional.ofNullable(null);
Review Comment:
```suggestion
this.volumeInfo = Optional.empty();
```
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestHddsVolume.java:
##########
@@ -369,6 +369,23 @@ public void testDbStoreClosedOnBadVolumeWithDbVolumes()
throws IOException {
assertEquals(0, DatanodeStoreCache.getInstance().size());
}
+ @Test
+ public void testFailedVolumeSpace() throws IOException {
+
+ // Build failed volume
+ HddsVolume volume = volumeBuilder.failedVolume(true).build();
+
+ // In case of failed volume all stats should return 0.
+ assertEquals(0, volume.getVolumeInfoStats().getUsed());
+ assertEquals(0, volume.getVolumeInfoStats().getAvailable());
+ assertEquals(0, volume.getVolumeInfoStats().getCapacity());
+ assertEquals(0, volume.getVolumeInfoStats().getReserved());
+ assertEquals(0, volume.getVolumeInfoStats().getTotalCapacity());
Review Comment:
NIt: please store `volume.getVolumeInfoStats()` as local variable.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/StorageVolume.java:
##########
@@ -461,15 +468,15 @@ public ConfigurationSource getConf() {
public void failVolume() {
setState(VolumeState.FAILED);
- if (volumeInfo != null) {
- volumeInfo.shutdownUsageThread();
+ if (volumeInfo.isPresent()) {
+ volumeInfo.get().shutdownUsageThread();
}
}
public void shutdown() {
setState(VolumeState.NON_EXISTENT);
- if (volumeInfo != null) {
- volumeInfo.shutdownUsageThread();
+ if (volumeInfo.isPresent()) {
+ volumeInfo.get().shutdownUsageThread();
}
Review Comment:
```suggestion
volumeInfo.ifPresent(VolumeInfo::shutdownUsageThread);
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/StorageVolume.java:
##########
@@ -417,8 +424,8 @@ public VolumeSet getVolumeSet() {
}
public StorageType getStorageType() {
- if (this.volumeInfo != null) {
- return this.volumeInfo.getStorageType();
+ if (volumeInfo.isPresent()) {
+ return volumeInfo.get().getStorageType();
}
return StorageType.DEFAULT;
Review Comment:
Use `volumeInfo.map(...).orElse(...)` here, too.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/StorageVolume.java:
##########
@@ -393,22 +397,25 @@ public String getWorkingDir() {
}
public void refreshVolumeInfo() {
- volumeInfo.refreshNow();
+
+ if (volumeInfo.isPresent()) {
+ volumeInfo.get().refreshNow();
+ }
}
- public VolumeInfo getVolumeInfo() {
+ public Optional<VolumeInfo> getVolumeInfo() {
return this.volumeInfo;
}
public void incrementUsedSpace(long usedSpace) {
- if (this.volumeInfo != null) {
- this.volumeInfo.incrementUsedSpace(usedSpace);
+ if (volumeInfo.isPresent()) {
+ volumeInfo.get().incrementUsedSpace(usedSpace);
}
Review Comment:
Please use `volumeInfo.ifPresent(...)`.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/StorageVolume.java:
##########
@@ -461,15 +468,15 @@ public ConfigurationSource getConf() {
public void failVolume() {
setState(VolumeState.FAILED);
- if (volumeInfo != null) {
- volumeInfo.shutdownUsageThread();
+ if (volumeInfo.isPresent()) {
+ volumeInfo.get().shutdownUsageThread();
}
Review Comment:
Please use `volumeInfo.ifPresent(...)`.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/StorageVolume.java:
##########
@@ -393,22 +397,25 @@ public String getWorkingDir() {
}
public void refreshVolumeInfo() {
- volumeInfo.refreshNow();
+
+ if (volumeInfo.isPresent()) {
+ volumeInfo.get().refreshNow();
+ }
Review Comment:
Please use `volumeInfo.ifPresent(...)`.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/StorageVolume.java:
##########
@@ -393,22 +397,25 @@ public String getWorkingDir() {
}
public void refreshVolumeInfo() {
- volumeInfo.refreshNow();
+
+ if (volumeInfo.isPresent()) {
+ volumeInfo.get().refreshNow();
+ }
}
- public VolumeInfo getVolumeInfo() {
+ public Optional<VolumeInfo> getVolumeInfo() {
return this.volumeInfo;
}
public void incrementUsedSpace(long usedSpace) {
- if (this.volumeInfo != null) {
- this.volumeInfo.incrementUsedSpace(usedSpace);
+ if (volumeInfo.isPresent()) {
+ volumeInfo.get().incrementUsedSpace(usedSpace);
}
}
public void decrementUsedSpace(long reclaimedSpace) {
- if (this.volumeInfo != null) {
- this.volumeInfo.decrementUsedSpace(reclaimedSpace);
+ if (volumeInfo.isPresent()) {
+ volumeInfo.get().decrementUsedSpace(reclaimedSpace);
}
Review Comment:
Please use `volumeInfo.ifPresent(...)`.
--
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]
