siddhantsangwan commented on code in PR #8915:
URL: https://github.com/apache/ozone/pull/8915#discussion_r2406174712
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/replication/TestSendContainerRequestHandler.java:
##########
@@ -189,12 +203,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() {
+ long containerId1 = 1;
+ long containerId2 = 2;
+ long overallocatedSize = 10L * 1024L * 1024L * 1024L; // 10GB
Review Comment:
`overallocatedSize` should be assigned using `containerMaxSize`, example
`containerMaxSize * 2`. If in the future the default container size changes,
this test should still work.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/ContainerImporter.java:
##########
@@ -146,11 +146,12 @@ private static void deleteFileQuietely(Path tarFilePath) {
}
}
- HddsVolume chooseNextVolume() throws IOException {
+ HddsVolume chooseNextVolume(long spaceToReserve) throws IOException {
// Choose volume that can hold both container in tmp and dest directory
+ LOG.info("Choosing volume to reserve space : {}", spaceToReserve);
Review Comment:
Let's make this a debug log, it could be logged too many times.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/GrpcContainerUploader.java:
##########
@@ -45,17 +47,29 @@ public class GrpcContainerUploader implements
ContainerUploader {
private final SecurityConfig securityConfig;
private final CertificateClient certClient;
+ private final ContainerController containerController;
public GrpcContainerUploader(
- ConfigurationSource conf, CertificateClient certClient) {
+ ConfigurationSource conf, CertificateClient certClient,
+ ContainerController containerController) {
this.certClient = certClient;
+ this.containerController = containerController;
securityConfig = new SecurityConfig(conf);
}
@Override
public OutputStream startUpload(long containerId, DatanodeDetails target,
- CompletableFuture<Void> callback, CopyContainerCompression compression)
- throws IOException {
+ CompletableFuture<Void> callback, CopyContainerCompression compression)
throws IOException {
+
+ // Get container size from local datanode instead of using passed
replicateSize
+ Long containerSize = null;
+ Container<?> container = containerController.getContainer(containerId);
+ if (container != null) {
+ LOG.info("Starting upload of container {} to {} with size {}",
Review Comment:
This should also be a debug log.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/replication/TestContainerReplication.java:
##########
@@ -157,6 +170,68 @@ void pushUnknownContainer() throws Exception {
ReplicationSupervisor::getReplicationFailureCount);
}
+ /**
+ * Provides stream of different container sizes for tests.
+ */
+ public static Stream<Arguments> sizeProvider() {
+ return Stream.of(
+ Arguments.of("Normal 2MB", 2L * 1024L * 1024L),
+ Arguments.of("Overallocated 6MB", 6L * 1024L * 1024L)
+ );
+ }
+
+ /**
+ * Tests push replication of a container with over-allocated size.
+ * The target datanode will need to reserve double the container size,
+ * which is greater than the configured max container size.
+ */
+ @ParameterizedTest(name = "for {0}")
+ @MethodSource("sizeProvider")
+ void testPushWithOverAllocatedContainer(String testName, Long containerSize)
Review Comment:
This can potentially be a flaky test because it tries to over allocate a
container. A container is supposed to close when it's about to reach its limit,
but there's a delay there which this test is relying on. It's good enough for
now, but we should track that this test isn't failing intermittently in future
CI runs. The fix would be to not try to over allocate it.
--
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]
