Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import org.apache.hadoop.hdds.HddsUtils;
import org.apache.hadoop.hdds.client.BlockID;
import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.hdds.fs.SpaceUsageSource;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerCommandRequestProto;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerCommandResponseProto;
Expand Down Expand Up @@ -616,10 +615,7 @@ private boolean isVolumeFull(Container container) {
.orElse(Boolean.FALSE);
if (isOpen) {
HddsVolume volume = container.getContainerData().getVolume();
SpaceUsageSource usage = volume.getCurrentUsage();
long volumeFreeSpaceToSpare = volume.getFreeSpaceToSpare(usage.getCapacity());
return !VolumeUsage.hasVolumeEnoughSpace(usage.getAvailable(), volume.getCommittedBytes(), 0,
volumeFreeSpaceToSpare);
return VolumeUsage.getUsableSpace(volume.getReport()) <= 0;
}
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.hadoop.ozone.container.common.impl;

import java.io.IOException;
import net.jcip.annotations.Immutable;
import org.apache.hadoop.fs.StorageType;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.MetadataStorageReportProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.StorageReportProto;
Expand All @@ -28,8 +29,8 @@
* Storage location stats of datanodes that provide back store for containers.
*
*/
public final class StorageLocationReport implements
StorageLocationReportMXBean {
@Immutable
public final class StorageLocationReport implements StorageLocationReportMXBean {

private final String id;
private final boolean failed;
Expand All @@ -41,19 +42,16 @@ public final class StorageLocationReport implements
private final StorageType storageType;
private final String storageLocation;

@SuppressWarnings("checkstyle:parameternumber")
private StorageLocationReport(String id, boolean failed, long capacity,
long scmUsed, long remaining, long committed, long freeSpaceToSpare,
StorageType storageType, String storageLocation) {
this.id = id;
this.failed = failed;
this.capacity = capacity;
this.scmUsed = scmUsed;
this.remaining = remaining;
this.committed = committed;
this.freeSpaceToSpare = freeSpaceToSpare;
this.storageType = storageType;
this.storageLocation = storageLocation;
private StorageLocationReport(Builder builder) {
this.id = builder.id;
this.failed = builder.failed;
this.capacity = builder.capacity;
this.scmUsed = builder.scmUsed;
this.remaining = builder.remaining;
this.committed = builder.committed;
this.freeSpaceToSpare = builder.freeSpaceToSpare;
this.storageType = builder.storageType;
this.storageLocation = builder.storageLocation;
}

@Override
Expand Down Expand Up @@ -304,6 +302,10 @@ public Builder setFailed(boolean failedValue) {
return this;
}

public boolean isFailed() {
return failed;
}

/**
* Sets the capacity of volume.
*
Expand All @@ -314,6 +316,11 @@ public Builder setCapacity(long capacityValue) {
this.capacity = capacityValue;
return this;
}

public long getCapacity() {
return capacity;
}

/**
* Sets the scmUsed Value.
*
Expand Down Expand Up @@ -387,8 +394,7 @@ public Builder setStorageLocation(String storageLocationValue) {
* @return StorageLocationReport
*/
public StorageLocationReport build() {
return new StorageLocationReport(id, failed, capacity, scmUsed,
remaining, committed, freeSpaceToSpare, storageType, storageLocation);
return new StorageLocationReport(this);
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import java.util.HashMap;
import java.util.Map;
import java.util.function.Predicate;
import org.apache.hadoop.hdds.fs.SpaceUsageSource;
import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport;

/**
* Filter for selecting volumes with enough space for a new container.
Expand All @@ -39,19 +39,14 @@ public AvailableSpaceFilter(long requiredSpace) {

@Override
public boolean test(HddsVolume vol) {
SpaceUsageSource usage = vol.getCurrentUsage();
long volumeCapacity = usage.getCapacity();
long free = usage.getAvailable();
long committed = vol.getCommittedBytes();
long available = free - committed;
long volumeFreeSpaceToSpare = vol.getFreeSpaceToSpare(volumeCapacity);
boolean hasEnoughSpace = VolumeUsage.hasVolumeEnoughSpace(free, committed,
requiredSpace, volumeFreeSpaceToSpare);
StorageLocationReport report = vol.getReport();
long available = VolumeUsage.getUsableSpace(report);
boolean hasEnoughSpace = available > requiredSpace;

mostAvailableSpace = Math.max(available, mostAvailableSpace);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall mostAvailableSpace consider to deduct volumeFreeSpaceToSpare too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ChenSammi for the review, that's a good idea.

I have updated the patch:

  • Refactor hasVolumeEnoughSpace to getUsableSpace, which does not accept requiredSpace parameter, rather returns the calculated space. The caller can compare, and further use if needed. Since this value has volumeFreeSpaceToSpare deducted, mostAvailable now does, too. This also allows removing all calculation from AvailableSpaceFilter.
  • Change the AvailableSpace helper class to get values from the report. In a follow-up I think we can get rid of this helper class, and change the map to store the report directly (need to implement toString() in StorageLocationReport).

Converted to draft until tests pass in CI.

Copy link
Contributor

@ChenSammi ChenSammi Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not change the fullVolumes to Map<HddsVolume, Long> and save available directly? I would suggest remove AvailableSpace right in this patch if there is already a refactor plan, since AvailableSpace is only used internally by AvailableSpaceFilter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not change the fullVolumes to Map<HddsVolume, Long> and save available directly?

I think providing more information about full volumes (free/used/committed/reserved/etc.) is useful to help understand why a volume does not have space.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it makes sense. Do you like to do the AvailableSpace removal in this patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to do it as part of HDDS-12721.


if (!hasEnoughSpace) {
fullVolumes.put(vol, new AvailableSpace(free, committed));
fullVolumes.put(vol, new AvailableSpace(report));
}

return hasEnoughSpace;
Expand All @@ -72,18 +67,16 @@ public String toString() {
}

private static class AvailableSpace {
private final long free;
private final long committed;
private final StorageLocationReport report;

AvailableSpace(long free, long committed) {
this.free = free;
this.committed = committed;
AvailableSpace(StorageLocationReport report) {
this.report = report;
}

@Override
public String toString() {
return "free: " + free +
", committed: " + committed;
return "free: " + report.getRemaining() +
", committed: " + report.getCommitted();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.hdds.upgrade.HDDSLayoutFeature;
import org.apache.hadoop.hdfs.server.datanode.checker.VolumeCheckResult;
import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport;
import org.apache.hadoop.ozone.container.common.utils.DatanodeStoreCache;
import org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil;
import org.apache.hadoop.ozone.container.common.utils.RawDB;
Expand Down Expand Up @@ -179,6 +180,16 @@ public VolumeInfoMetrics getVolumeInfoStats() {
return volumeInfoMetrics;
}

@Override
protected StorageLocationReport.Builder reportBuilder() {
StorageLocationReport.Builder builder = super.reportBuilder();
if (!builder.isFailed()) {
builder.setCommitted(getCommittedBytes())
.setFreeSpaceToSpare(getFreeSpaceToSpare(builder.getCapacity()));
}
return builder;
}

@Override
public void failVolume() {
super.failVolume();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,17 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.EnumMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import org.apache.hadoop.fs.StorageType;
import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.hdds.fs.SpaceUsageCheckFactory;
import org.apache.hadoop.hdds.fs.SpaceUsageSource;
import org.apache.hadoop.hdds.utils.HddsServerUtil;
import org.apache.hadoop.hdfs.server.datanode.StorageLocation;
import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport;
Expand Down Expand Up @@ -455,68 +452,15 @@ public boolean hasEnoughVolumes() {
}

public StorageLocationReport[] getStorageReport() {
boolean failed;
this.readLock();
try {
StorageLocationReport[] reports = new StorageLocationReport[volumeMap
.size() + failedVolumeMap.size()];
StorageLocationReport[] reports = new StorageLocationReport[volumeMap.size() + failedVolumeMap.size()];
int counter = 0;
StorageVolume volume;
for (Map.Entry<String, StorageVolume> entry : volumeMap.entrySet()) {
volume = entry.getValue();
Optional<VolumeUsage> volumeUsage = volume.getVolumeUsage();
long scmUsed = 0;
long remaining = 0;
long capacity = 0;
long committed = 0;
long spare = 0;
String rootDir = volume.getVolumeRootDir();
failed = true;
if (volumeUsage.isPresent()) {
try {
SpaceUsageSource usage = volumeUsage.get().getCurrentUsage();
scmUsed = usage.getUsedSpace();
remaining = usage.getAvailable();
capacity = usage.getCapacity();
HddsVolume hddsVolume = volume instanceof HddsVolume ? (HddsVolume) volume : null;
committed = hddsVolume != null ? hddsVolume.getCommittedBytes() : 0;
spare = hddsVolume != null ? hddsVolume.getFreeSpaceToSpare(capacity) : 0;
failed = false;
} catch (UncheckedIOException ex) {
LOG.warn("Failed to get scmUsed and remaining for container " +
"storage location {}", rootDir, ex);
// reset scmUsed and remaining if df/du failed.
scmUsed = 0;
remaining = 0;
capacity = 0;
}
}

StorageLocationReport.Builder builder =
StorageLocationReport.newBuilder();
builder.setStorageLocation(rootDir)
.setId(volume.getStorageID())
.setFailed(failed)
.setCapacity(capacity)
.setRemaining(remaining)
.setScmUsed(scmUsed)
.setCommitted(committed)
.setFreeSpaceToSpare(spare)
.setStorageType(volume.getStorageType());
StorageLocationReport r = builder.build();
reports[counter++] = r;
for (StorageVolume volume : volumeMap.values()) {
reports[counter++] = volume.getReport();
}
for (Map.Entry<String, StorageVolume> entry
: failedVolumeMap.entrySet()) {
volume = entry.getValue();
StorageLocationReport.Builder builder = StorageLocationReport
.newBuilder();
builder.setStorageLocation(volume.getStorageDir()
.getAbsolutePath()).setId(volume.getStorageID()).setFailed(true)
.setCapacity(0).setRemaining(0).setScmUsed(0).setStorageType(
volume.getStorageType());
StorageLocationReport r = builder.build();
reports[counter++] = r;
for (StorageVolume volume : failedVolumeMap.values()) {
reports[counter++] = volume.getReport();
}
return reports;
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.apache.hadoop.hdfs.server.datanode.checker.VolumeCheckResult;
import org.apache.hadoop.ozone.common.InconsistentStorageStateException;
import org.apache.hadoop.ozone.container.common.helpers.DatanodeVersionFile;
import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport;
import org.apache.hadoop.ozone.container.common.statemachine.DatanodeConfiguration;
import org.apache.hadoop.ozone.container.common.utils.DiskCheckUtil;
import org.apache.hadoop.ozone.container.common.utils.StorageVolumeUtil;
Expand Down Expand Up @@ -454,6 +455,27 @@ public SpaceUsageSource getCurrentUsage() {
.orElse(SpaceUsageSource.UNKNOWN);
}

protected StorageLocationReport.Builder reportBuilder() {
StorageLocationReport.Builder builder = StorageLocationReport.newBuilder()
.setFailed(isFailed())
.setId(getStorageID())
.setStorageLocation(volumeRoot)
.setStorageType(storageType);

if (!builder.isFailed()) {
SpaceUsageSource usage = getCurrentUsage();
builder.setCapacity(usage.getCapacity())
.setRemaining(usage.getAvailable())
.setScmUsed(usage.getUsedSpace());
}

return builder;
}

public StorageLocationReport getReport() {
return reportBuilder().build();
}

public File getStorageDir() {
return this.storageDir;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import org.apache.hadoop.hdds.fs.CachingSpaceUsageSource;
import org.apache.hadoop.hdds.fs.SpaceUsageCheckParams;
import org.apache.hadoop.hdds.fs.SpaceUsageSource;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.StorageReportProto;
import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport;
import org.apache.ratis.util.Preconditions;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -177,11 +179,17 @@ public long getReservedInBytes() {
return reservedInBytes;
}

public static boolean hasVolumeEnoughSpace(long volumeAvailableSpace,
long volumeCommittedBytesCount,
long requiredSpace,
long volumeFreeSpaceToSpare) {
return (volumeAvailableSpace - volumeCommittedBytesCount - volumeFreeSpaceToSpare) > requiredSpace;
private static long getUsableSpace(
long available, long committed, long minFreeSpace) {
return available - committed - minFreeSpace;
}

public static long getUsableSpace(StorageReportProto report) {
return getUsableSpace(report.getRemaining(), report.getCommitted(), report.getFreeSpaceToSpare());
}

public static long getUsableSpace(StorageLocationReport report) {
return getUsableSpace(report.getRemaining(), report.getCommitted(), report.getFreeSpaceToSpare());
}

private static long getReserved(ConfigurationSource conf, String rootDir,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,7 @@ public static boolean hasEnoughSpace(DatanodeDetails datanodeDetails,

if (dataSizeRequired > 0) {
for (StorageReportProto reportProto : datanodeInfo.getStorageReports()) {
if (VolumeUsage.hasVolumeEnoughSpace(reportProto.getRemaining(),
reportProto.getCommitted(), dataSizeRequired,
reportProto.getFreeSpaceToSpare())) {
if (VolumeUsage.getUsableSpace(reportProto) > dataSizeRequired) {
enoughForData = true;
break;
}
Expand Down