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 2021/05/13 08:47:20 UTC
[GitHub] [ozone] guihecheng opened a new pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
guihecheng opened a new pull request #2246:
URL: https://github.com/apache/ozone/pull/2246
## What changes were proposed in this pull request?
When use placement policy to choose datanodes, we should check whether a datanode has a volume that
has enough space to hold the container, not check the space from all volumes together, because a container
could only be on a single volume not spread across volumes.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5209
## How was this patch tested?
extended existing ut.
--
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.
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] guihecheng edited a comment on pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
guihecheng edited a comment on pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#issuecomment-841044492
@ChenSammi updated, thanks~
cc @bshashikant , please help review this since it can be related to datanode volume handling issues.
--
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.
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] ChenSammi commented on a change in pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#discussion_r633490167
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
+ DatanodeInfo datanodeInfo = (DatanodeInfo) datanodeDetails;
+ for (StorageReportProto reportProto : datanodeInfo.getStorageReports()) {
+ if (reportProto.getRemaining() > sizeRequired) {
+ return true;
+ }
+ }
+ }
+ return false;
Review comment:
I think we need to distinguish two "false" cases here, one is datanodeDetails is not a DatanodeInfo, another is this DN doesn't have enough space. The latter case is normal, but the formal case indidates that current solution is not perfect. It would be better if there is a WARN or INFO log so that we can find the issue quickly.
--
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.
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] guihecheng commented on a change in pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#discussion_r634003073
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
+ DatanodeInfo datanodeInfo = (DatanodeInfo) datanodeDetails;
+ for (StorageReportProto reportProto : datanodeInfo.getStorageReports()) {
+ if (reportProto.getRemaining() > sizeRequired) {
+ return true;
+ }
+ }
+ }
+ return false;
Review comment:
Added a precondition check instead of a LOG, since it is not directly related to the service provided here.
--
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.
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] JacksonYao287 commented on a change in pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#discussion_r632413608
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
Review comment:
Thanks @guihecheng for this work!
i have a little question, is there existing a situation that a `datanodeDetails` is not an instance of `DatanodeInfo`, but the corresponding datanode does have a volume that has enough space for the requirement?
if no, then every `datanodeDetails` is an instance of `DatanodeInfo`, so can we deprecate `datanodeDetails` and only use `DatanodeInfo`?
if yes, will this lead to a misjudgegment?
--
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.
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] guihecheng commented on pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
guihecheng commented on pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#issuecomment-840457884
Hi @ChenSammi please help review this one, I'll check the CI failures, not related at first glance, thanks~
--
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.
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] guihecheng commented on pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
guihecheng commented on pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#issuecomment-840488877
@ChenSammi ah, then I shall put the logic directly into hasEnoughSpace and avoid adding a new API, thanks~
--
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.
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] ChenSammi commented on a change in pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#discussion_r633490167
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
+ DatanodeInfo datanodeInfo = (DatanodeInfo) datanodeDetails;
+ for (StorageReportProto reportProto : datanodeInfo.getStorageReports()) {
+ if (reportProto.getRemaining() > sizeRequired) {
+ return true;
+ }
+ }
+ }
+ return false;
Review comment:
I think we need to distinguish two "false" cases here, one is datanodeDetails is not a DatanodeInfo, another is this DN doesn't have enough space. The latter case is normal, but the formal case indidates that something unexpected. It would be better if there is a WARN log so that we can find the issue quickly.
--
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.
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] guihecheng commented on a change in pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#discussion_r633182776
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
Review comment:
@JacksonYao287 oh, thanks for reviewing, here the check is just to suppress downcasting warning actually.
AFAIK, SCMNodeManager get nodes from SCMNodeStateManger and these datanodesDetails shall always be datanodeInfos, the situation will not be hit.
We could do some refactoring for usage of `DatanodeDetails` and `DatanodeInfo` if needed, but not here I think.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
+ DatanodeInfo datanodeInfo = (DatanodeInfo) datanodeDetails;
+ for (StorageReportProto reportProto : datanodeInfo.getStorageReports()) {
+ if (reportProto.getRemaining() > sizeRequired) {
+ return true;
+ }
+ }
+ }
+ return false;
Review comment:
@ChenSammi sure, we could add a WARN for the first 'false' case, if hit then it implies that we should find a better solution here.
--
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.
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] ChenSammi commented on pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#issuecomment-840464399
Hi @guihecheng , we have leverage the DatanodeInfo value. It seems there is no need to add new API in NodeManager. You can refer to these piece of codes,
final DatanodeInfo datanodeInfo = nodeStateManager
.getNode(datanodeDetails);
final List<StorageReportProto> storageReportProtos = datanodeInfo
.getStorageReports();
for (StorageReportProto reportProto : storageReportProtos) {
capacity += reportProto.getCapacity();
used += reportProto.getScmUsed();
remaining += reportProto.getRemaining();
}
--
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.
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] ChenSammi commented on a change in pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#discussion_r633490167
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
+ DatanodeInfo datanodeInfo = (DatanodeInfo) datanodeDetails;
+ for (StorageReportProto reportProto : datanodeInfo.getStorageReports()) {
+ if (reportProto.getRemaining() > sizeRequired) {
+ return true;
+ }
+ }
+ }
+ return false;
Review comment:
I think we need to distinguish two "false" cases here, one is datanodeDetails is not a DatanodeInfo, another is this DN doesn't have enough space. The latter case is normal, but the formal case indidates that current solution is not perfect. It would be better if there is a WARN log so that we can find the issue quickly.
--
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.
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] ChenSammi merged pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
ChenSammi merged pull request #2246:
URL: https://github.com/apache/ozone/pull/2246
--
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.
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] guihecheng commented on a change in pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#discussion_r633182776
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
Review comment:
@JacksonYao287 oh, thanks for reviewing, here the check is just to suppress downcasting warning actually.
AFAIK, SCMNodeManager get nodes from SCMNodeStateManger and these datanodesDetails shall always be datanodeInfos, the situation will not be hit.
We could do some refactoring for usage of `DatanodeDetails` and `DatanodeInfo` if needed, but not here I think.
--
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.
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] JacksonYao287 commented on a change in pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#discussion_r633252992
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
Review comment:
LGTM. +1
--
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.
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] ChenSammi commented on pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#issuecomment-843799967
+1. Thanks @guihecheng 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.
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] ChenSammi commented on a change in pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#discussion_r633490167
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
+ DatanodeInfo datanodeInfo = (DatanodeInfo) datanodeDetails;
+ for (StorageReportProto reportProto : datanodeInfo.getStorageReports()) {
+ if (reportProto.getRemaining() > sizeRequired) {
+ return true;
+ }
+ }
+ }
+ return false;
Review comment:
I think we need to distinguish two "false" cases here, one is datanodeDetails is not a DatanodeInfo, another is this DN doesn't have enough space. The latter case is normal, but the formal case indidates that current solution is not perfect. It would be better if there is a WARN or INFO log so that we can find the issue quickly.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
+ DatanodeInfo datanodeInfo = (DatanodeInfo) datanodeDetails;
+ for (StorageReportProto reportProto : datanodeInfo.getStorageReports()) {
+ if (reportProto.getRemaining() > sizeRequired) {
+ return true;
+ }
+ }
+ }
+ return false;
Review comment:
I think we need to distinguish two "false" cases here, one is datanodeDetails is not a DatanodeInfo, another is this DN doesn't have enough space. The latter case is normal, but the formal case indidates that current solution is not perfect. It would be better if there is a WARN log so that we can find the issue quickly.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
+ DatanodeInfo datanodeInfo = (DatanodeInfo) datanodeDetails;
+ for (StorageReportProto reportProto : datanodeInfo.getStorageReports()) {
+ if (reportProto.getRemaining() > sizeRequired) {
+ return true;
+ }
+ }
+ }
+ return false;
Review comment:
I think we need to distinguish two "false" cases here, one is datanodeDetails is not a DatanodeInfo, another is this DN doesn't have enough space. The latter case is normal, but the formal case indidates that something unexpected. It would be better if there is a WARN log so that we can find the issue quickly.
--
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.
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] guihecheng commented on pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
guihecheng commented on pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#issuecomment-841044492
cc @bshashikant , please help review this since it can be related to datanode volume handling issues.
--
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.
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] JacksonYao287 commented on a change in pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#discussion_r633252992
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
Review comment:
LGTM. +1
--
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.
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] guihecheng commented on a change in pull request #2246: HDDS-5209. Datanode hasEnoughSpace check should apply on volume instead of global DN
Posted by GitBox <gi...@apache.org>.
guihecheng commented on a change in pull request #2246:
URL: https://github.com/apache/ozone/pull/2246#discussion_r633493923
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java
##########
@@ -165,9 +166,15 @@ public ConfigurationSource getConf() {
*/
public boolean hasEnoughSpace(DatanodeDetails datanodeDetails,
long sizeRequired) {
- SCMNodeMetric nodeMetric = nodeManager.getNodeStat(datanodeDetails);
- return (nodeMetric != null) && (nodeMetric.get() != null)
- && nodeMetric.get().getRemaining().hasResources(sizeRequired);
+ if (datanodeDetails instanceof DatanodeInfo) {
+ DatanodeInfo datanodeInfo = (DatanodeInfo) datanodeDetails;
+ for (StorageReportProto reportProto : datanodeInfo.getStorageReports()) {
+ if (reportProto.getRemaining() > sizeRequired) {
+ return true;
+ }
+ }
+ }
+ return false;
Review comment:
@ChenSammi sure, we could add a WARN for the first 'false' case, if hit then it implies that we should find a better solution here.
--
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.
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