errose28 commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r906616072
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java:
##########
@@ -183,8 +204,9 @@ private VolumeInfo(Builder b) throws IOException {
SpaceUsageCheckParams checkParams =
usageCheckFactory.paramsFor(root);
+ this.usage = new VolumeUsage(checkParams);
this.reservedInBytes = getReserved(b.conf);
- this.usage = new VolumeUsage(checkParams, reservedInBytes);
+ this.usage.setReserved(reservedInBytes);
Review Comment:
Why can't we pass the reserved bytes to the constructor like before?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java:
##########
@@ -134,25 +135,45 @@ public VolumeInfo build() throws IOException {
}
private long getReserved(ConfigurationSource conf) {
+ final float defaultValue = Float.MAX_VALUE;
Review Comment:
I think `defaultValue` should be replaced with a constant from
`ScmConfigKeys` as mentioned above. In the check on line 144, it does not
matter whether the config ended up as 0 from the default value or an explicit
user setting. We can easily override it with the reserveList in this case. I
think there should only be an error if percentage != 0 and reserveList.size() >
0.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -214,6 +214,8 @@ public final class ScmConfigKeys {
public static final String HDDS_DATANODE_DIR_KEY = "hdds.datanode.dir";
public static final String HDDS_DATANODE_DIR_DU_RESERVED =
"hdds.datanode.dir.du.reserved";
+ public static final String HDDS_DATANODE_VOLUME_RESERVED =
+ "hdds.datanode.volume.reserved";
Review Comment:
```suggestion
public static final String HDDS_DATANODE_DIR_DU_RESERVED_PERCENT =
"hdds.datanode.dir.du.reserved.percent";
```
I think this name will be clearer. It distinguishes it from the old pair
based config, and uses the common hdds.datanode.dir prefix already used for
other volume configs.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -214,6 +214,8 @@ public final class ScmConfigKeys {
public static final String HDDS_DATANODE_DIR_KEY = "hdds.datanode.dir";
public static final String HDDS_DATANODE_DIR_DU_RESERVED =
"hdds.datanode.dir.du.reserved";
+ public static final String HDDS_DATANODE_VOLUME_RESERVED =
+ "hdds.datanode.volume.reserved";
Review Comment:
We should also put the default value of 0 as a constant here.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java:
##########
@@ -134,25 +135,45 @@ public VolumeInfo build() throws IOException {
}
private long getReserved(ConfigurationSource conf) {
+ final float defaultValue = Float.MAX_VALUE;
+ float percentage = conf.getFloat(HDDS_DATANODE_VOLUME_RESERVED,
+ defaultValue);
Collection<String> reserveList = conf.getTrimmedStringCollection(
HDDS_DATANODE_DIR_DU_RESERVED);
- for (String reserve : reserveList) {
- String[] words = reserve.split(":");
- if (words.length < 2) {
- LOG.error("Reserved space should config in pair, but current is {}",
- reserve);
- continue;
- }
+ //Both the configs are set. Log it and return 0
+ if (reserveList.size() > 0 && percentage != defaultValue) {
+ LOG.error("Both {} and {} are set. Set either one, not both. " +
+ "Setting reserved to 0.", HDDS_DATANODE_VOLUME_RESERVED,
+ HDDS_DATANODE_DIR_DU_RESERVED);
+ return 0;
Review Comment:
This configuration error might be more user friendly if made fatal. If the
admin misses the log message they won't know they have made a mistake until
their disks are full. I know this error handling was adapted from the old
config, but I think we should fix it here.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java:
##########
@@ -134,25 +135,45 @@ public VolumeInfo build() throws IOException {
}
private long getReserved(ConfigurationSource conf) {
+ final float defaultValue = Float.MAX_VALUE;
+ float percentage = conf.getFloat(HDDS_DATANODE_VOLUME_RESERVED,
+ defaultValue);
Collection<String> reserveList = conf.getTrimmedStringCollection(
HDDS_DATANODE_DIR_DU_RESERVED);
- for (String reserve : reserveList) {
- String[] words = reserve.split(":");
- if (words.length < 2) {
- LOG.error("Reserved space should config in pair, but current is {}",
- reserve);
- continue;
- }
+ //Both the configs are set. Log it and return 0
+ if (reserveList.size() > 0 && percentage != defaultValue) {
+ LOG.error("Both {} and {} are set. Set either one, not both. " +
+ "Setting reserved to 0.", HDDS_DATANODE_VOLUME_RESERVED,
+ HDDS_DATANODE_DIR_DU_RESERVED);
+ return 0;
Review Comment:
Same for all other errors below.
--
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]
