You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/06/18 04:13:43 UTC
[GitHub] [ozone] aswinshakil opened a new pull request, #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
aswinshakil opened a new pull request, #3532:
URL: https://github.com/apache/ozone/pull/3532
## What changes were proposed in this pull request?
Currently, We can reserve space on a volume using `hdds.datanode.dir.du.reserved`, which is key-value pair where we specify the volume and the space that should be reserved in that volume (i.e data1:5000MB). If there are multiple volumes we have put a key-value pair of volume:reserved for each volume. This PR aims to create a configuration that sets the percentage of volume reserved for all the volumes in the Datanode.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6901
## How was this patch tested?
Patch was tested manually using docker and unit tests.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r906912878
##########
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) {
Review Comment:
Also, please add a basic unit test for this method.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r906421729
##########
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:
`Float.MAX_VALUE` is used as an arbitrary value, to check whether the config `HDDS_DATANODE_VOLUME_RESERVED` is set.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r911278022
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfo.java:
##########
@@ -149,12 +161,27 @@ private long getReserved(ConfigurationSource conf) {
StorageSize size = StorageSize.parse(words[1].trim());
return (long) size.getUnit().toBytes(size.getValue());
} catch (Exception e) {
- LOG.error("Failed to parse StorageSize:{}", words[1].trim(), e);
+ LOG.error("Failed to parse StorageSize: {} ,Setting reserved " +
+ "space to zero", words[1].trim(), e);
Review Comment:
failure to parse should not return to 0 but try to see if the percentage setting is set.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r910446612
##########
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:
Thanks for the suggestion. I have updated the PR.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r911276517
##########
hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java:
##########
@@ -143,6 +143,18 @@ default Map<String, String> getPropsMatchPrefix(String keyPrefix) {
return configMap;
}
+ /**
+ * Checks if the property <value> is set.
+ * @param key The property name.
+ * @return true if the value is set else false.
+ */
+ default boolean isConfigured(String key) {
+ String configValue = get(key);
+ if (configValue != null) {
+ return true;
+ }
+ return false;
+ }
Review Comment:
```suggestion
default boolean isConfigured(String key) {
return get(key) != null;
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime merged pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
kerneltime merged PR #3532:
URL: https://github.com/apache/ozone/pull/3532
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r906912426
##########
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:
Here is a simpler version (by adding a new interface in `ConfigurationSource` interface
```
private long getReserved(ConfigurationSource conf) {
if (conf.isConfigured(HDDS_DATANODE_VOLUME_RESERVED) &&
conf.isConfigured(HDDS_DATANODE_DIR_DU_RESERVED)) {
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;
}
if (conf.isConfigured(HDDS_DATANODE_VOLUME_RESERVED)) {
final float defaultValue = 0;
float percentage = conf.getFloat(HDDS_DATANODE_VOLUME_RESERVED,
defaultValue);
if (0 <= percentage && percentage <= 1) {
return (long) Math.ceil(this.usage.getCapacity() * percentage);
}
//If it comes here then the percentage is not between 0-1.
LOG.error("The value of {} should be between 0 to 1. Defaulting to 0.",
HDDS_DATANODE_VOLUME_RESERVED);
return 0;
}
Collection<String> reserveList = conf.getTrimmedStringCollection(
HDDS_DATANODE_DIR_DU_RESERVED);
//Both the configs are set. Log it and return 0
if (reserveList.size() > 0) {
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;
}
if (words[0].trim().equals(rootDir)) {
try {
StorageSize size = StorageSize.parse(words[1].trim());
return (long) size.getUnit().toBytes(size.getValue());
} catch (Exception e) {
LOG.error("Failed to parse StorageSize:{}", words[1].trim(), e);
return 0;
}
}
}
}
return 0;
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r906513359
##########
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:
line 144 below reads funny.. why not just read the percent as default to 0 and simplify this method?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r906613803
##########
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:
Even if we default to 0, we need line 144 to check only one config is set and not both. Further simplifying the function makes it harder to read.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r907977206
##########
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:
Thank you for the suggeestion. I'll update the PR.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r907981685
##########
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:
The reason is we need `VolumeUsage` object first to calculate the `reservedBytes`. `VolumeUsage` gives the total capacity, from which the reserved bytes are calculated.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r907983338
##########
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) {
Review Comment:
`TestReservedVolumeSpace` is intended to test this method specifically.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r906912650
##########
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) {
Review Comment:
Should the behavior be to pick the max of the 2 values? Would be less problematic than not honoring any reserved space.
##########
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) {
Review Comment:
Should the behavior be to pick the max of the 2 values? Would be less problematic than not honoring any reserved space?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r908798413
##########
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:
I agree, preferring per volume over percentage would be nice. It is better to underutilize the disk due to the customer setting up reserved space incorrectly than to use up space that then needs to be moved away.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3532:
URL: https://github.com/apache/ozone/pull/3532#issuecomment-1180821027
Thank you @aswinshakil for the contribution.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r906411775
##########
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:
Why is default `Float.MAX_VALUE`
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #3532: HDDS-6901. Configure HDDS volume reserved as percentage of the volume space.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3532:
URL: https://github.com/apache/ozone/pull/3532#discussion_r907982514
##########
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) {
Review Comment:
The behavior should be set to only one config. It is either by each volume or for all volumes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org