peterxcli commented on code in PR #8915:
URL: https://github.com/apache/ozone/pull/8915#discussion_r2326401653
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/SendContainerRequestHandler.java:
##########
@@ -146,8 +155,8 @@ public void onCompleted() {
responseObserver.onError(t);
}
} finally {
- if (volume != null) {
- volume.incCommittedBytes(-importer.getDefaultReplicationSpace());
+ if (volume != null && spaceToReserve > 0) {
Review Comment:
Should we make `DownloadAndImportReplicator` follow the same condition?
The other is, if you wont `decrCommittedBytes` if spaceToReserve is zero,
then I think you should keep the chooseNextVolume as the same pattern.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/replication/TestSendContainerRequestHandler.java:
##########
@@ -87,6 +92,24 @@ void setup() throws IOException {
containerMaxSize = (long) conf.getStorageSize(
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE,
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.BYTES);
+
+ // Mock the space reservation logic to be independent of the
+ // importer's implementation details.
+ when(importer.getRequiredReplicationSpace(anyLong()))
+ .thenAnswer(invocation -> 2 * (long) invocation.getArgument(0));
+ when(importer.getDefaultReplicationSpace()).thenReturn(2 *
containerMaxSize);
Review Comment:
`importer` is a spy object not mocked one, so I think we dont need these.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/replication/TestSendContainerRequestHandler.java:
##########
@@ -191,12 +211,51 @@ public void
testSpaceReservedAndReleasedWhenOnCompletedFails() throws Exception
assertEquals(volume.getCommittedBytes(), initialCommittedBytes);
}
- private ContainerProtos.SendContainerRequest createRequest(long containerId,
ByteString data, int offset) {
- return ContainerProtos.SendContainerRequest.newBuilder()
- .setContainerID(containerId)
- .setData(data)
- .setOffset(offset)
- .setCompression(CopyContainerCompression.NO_COMPRESSION.toProto())
- .build();
+ /**
+ * Test that verifies the actual space calculation difference between
+ * overallocated containers and default containers.
+ */
+ @Test
+ public void testOverallocatedReservesMoreSpace() {
Review Comment:
```suggestion
public void testOverAllocatedReservesMoreSpace() {
```
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/replication/TestDownloadAndImportReplicator.java:
##########
@@ -75,19 +83,81 @@ void setup() throws IOException {
StorageVolume.VolumeType.DATA_VOLUME, null);
importer = new ContainerImporter(conf, containerSet,
mock(ContainerController.class), volumeSet, volumeChoosingPolicy);
+ importer = spy(importer);
downloader = mock(SimpleContainerDownloader.class);
replicator = new DownloadAndImportReplicator(conf, containerSet, importer,
downloader);
containerMaxSize = (long) conf.getStorageSize(
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE,
ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.BYTES);
+
+ // Mock the space reservation logic to be independent of the
+ // importer's implementation details.
+ when(importer.getRequiredReplicationSpace(anyLong()))
Review Comment:
ditto
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/DownloadAndImportReplicator.java:
##########
@@ -71,9 +71,20 @@ public void replicate(ReplicationTask task) {
LOG.info("Starting replication of container {} from {} using {}",
containerID, sourceDatanodes, compression);
HddsVolume targetVolume = null;
+
+ // Use replicate size from command if available, otherwise use default
+ long spaceReserved = 0;
+ Long replicateSize = task.getReplicateSize();
+ if (replicateSize != null) {
Review Comment:
> So is it possible that empty containers will be created?
I think yes
--
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]
