making review Changes

linkedin · Dec 20, 2024 · 7d0a130 · 7d0a130
1 parent 44f2ae6
commit 7d0a130
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 5 deletions.
diff --git a/ambry-protocol/src/main/java/com/github/ambry/protocol/FileCopyGetChunkRequest.java b/ambry-protocol/src/main/java/com/github/ambry/protocol/FileCopyGetChunkRequest.java
@@ -34,7 +34,7 @@ public FileCopyGetChunkRequest( short versionId, int correlationId,
       String clientId, PartitionId partitionId, String fileName, long startOffset, long sizeInBytes) {
     super(RequestOrResponseType.FileCopyGetChunkRequest, versionId, correlationId, clientId);
     if(partitionId == null || fileName.isEmpty() || startOffset < 0 || sizeInBytes < 0){
-      throw new IllegalArgumentException("PartitionId, FileName, StartOffset and SizeInBytes cannot be null or negative");
+      throw new IllegalArgumentException("PartitionId, FileName, StartOffset or SizeInBytes cannot be null or negative");
     }
     this.partitionId = partitionId;
     this.fileName = fileName;

diff --git a/ambry-protocol/src/main/java/com/github/ambry/protocol/FileCopyGetMetaDataRequest.java b/ambry-protocol/src/main/java/com/github/ambry/protocol/FileCopyGetMetaDataRequest.java
@@ -29,8 +29,11 @@ public class FileCopyGetMetaDataRequest extends RequestOrResponse{
   public FileCopyGetMetaDataRequest(short versionId, int correlationId, String clientId,
       PartitionId partitionId, String hostName) {
     super(RequestOrResponseType.FileCopyGetMetaDataRequest, versionId, correlationId, clientId);
-    if (partitionId == null || hostName.isEmpty()) {
-      throw new IllegalArgumentException("Partition and Host Name cannot be null");
+    if (partitionId == null) {
+      throw new IllegalArgumentException("Partition cannot be null");
+    }
+    if (hostName.isEmpty()){
+      throw new IllegalArgumentException("Host Name cannot be null");
     }
     this.partitionId = partitionId;
     this.hostName = hostName;

diff --git a/ambry-protocol/src/main/java/com/github/ambry/protocol/FileCopyGetMetaDataResponse.java b/ambry-protocol/src/main/java/com/github/ambry/protocol/FileCopyGetMetaDataResponse.java
@@ -38,15 +38,17 @@ public FileCopyGetMetaDataResponse(short versionId, int correlationId, String cl
   public static FileCopyGetMetaDataResponse readFrom(DataInputStream stream) throws IOException {
     RequestOrResponseType type = RequestOrResponseType.values()[stream.readShort()];
     if (type != RequestOrResponseType.FileCopyGetMetaDataResponse) {
-      throw new IllegalArgumentException("The type of request response is not compatible");
+      throw new IllegalArgumentException("The type of request response is not compatible. Expected : {}, Actual : {}" +
+          RequestOrResponseType.FileCopyGetMetaDataResponse + type);
     }
     short versionId = stream.readShort();
     int correlationId = stream.readInt();
     String clientId = Utils.readIntString(stream);
     ServerErrorCode errorCode = ServerErrorCode.values()[stream.readShort()];
 
     if(errorCode != ServerErrorCode.No_Error) {
-      return new FileCopyGetMetaDataResponse(versionId, correlationId, clientId, -1, new ArrayList<>(), errorCode);
+      //Setting the number of logfiles to 0 as there are no logfiles to be read.
+      return new FileCopyGetMetaDataResponse(versionId, correlationId, clientId, 0, new ArrayList<>(), errorCode);
     }
 
     int numberOfLogfiles = stream.readInt();

diff --git a/ambry-protocol/src/main/java/com/github/ambry/protocol/FileInfo.java b/ambry-protocol/src/main/java/com/github/ambry/protocol/FileInfo.java
@@ -19,6 +19,11 @@
 import java.io.IOException;
 import java.nio.charset.Charset;
 
+
+/**
+ * Contains the fileName and fileSizeInBytes for a local partition. This is used
+ * by LogInfo as part of filecopy metadata request.
+ */
 public class FileInfo {
   private String fileName;
   private long fileSizeInBytes;

diff --git a/ambry-protocol/src/main/java/com/github/ambry/protocol/LogInfo.java b/ambry-protocol/src/main/java/com/github/ambry/protocol/LogInfo.java
@@ -21,6 +21,11 @@
 import java.util.ArrayList;
 import java.util.List;
 
+
+/**
+ * Contains the fileName, fileSizeInBytes, indexFiles and bloomFilters for a local partition. This is used
+ * by filecopy metadata request.
+ */
 public class LogInfo {
   private String fileName;
   private long fileSizeInBytes;

diff --git a/ambry-protocol/src/test/java/com/github/ambry/protocol/RequestResponseTest.java b/ambry-protocol/src/test/java/com/github/ambry/protocol/RequestResponseTest.java
@@ -65,6 +65,7 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
+import org.mockito.Mock;
 
 import static com.github.ambry.account.Account.*;
 import static com.github.ambry.account.Container.*;
@@ -687,6 +688,22 @@ public void doFileCopyMetaDataRequestTest() throws IOException {
     Assert.assertEquals(fileMetadataRequestFromBytes.getPartitionId().toPathString(), "0");
 
     request.release();
+
+    try{
+      new FileCopyGetMetaDataRequest(requestVersionToUse, 111, "id1",
+          null, "host3");
+      Assert.fail("Should have thrown exception");
+    }catch (IllegalArgumentException e) {
+      //expected
+    }
+
+    try{
+      new FileCopyGetMetaDataRequest(requestVersionToUse, 111, "id1",
+          new MockPartitionId(), "");
+      Assert.fail("Should have thrown exception");
+    }catch (IllegalArgumentException e) {
+      //expected
+    }
   }
 
   @Test
@@ -755,6 +772,16 @@ public void doFileCopyMetaDataResponseTest() throws IOException{
     Assert.assertEquals(fileCopyProtocolMetaDataResponseTranformed.getLogInfos().get(1).getBloomFilters().get(0).getFileName(), "1_1_bloom");
     Assert.assertEquals(fileCopyProtocolMetaDataResponseTranformed.getLogInfos().get(1).getBloomFilters().get(0).getFileSizeInBytes(), 1040);
     response.release();
+
+
+    response =
+        new FileCopyGetMetaDataResponse(requestVersionToUse, 111, "id1", 2 ,
+            logInfoList, ServerErrorCode.IO_Error);
+    DataInputStream requestStream1 = serAndPrepForRead(response, -1, false);
+    FileCopyGetMetaDataResponse fileCopyProtocolGetChunkResponse =
+        FileCopyGetMetaDataResponse.readFrom(requestStream1);
+    Assert.assertEquals(fileCopyProtocolGetChunkResponse.getError(), ServerErrorCode.IO_Error);
+    response.release();
   }
 
   @Test
@@ -773,6 +800,39 @@ public void doFileCopyChunkRequestTest() throws IOException{
     Assert.assertEquals(request.getStartOffset(), 1000);
     Assert.assertEquals(request.getVersionId(), requestVersionToUse);
     request.release();
+
+    try{
+      new FileCopyGetChunkRequest(requestVersionToUse, 111, "id1",
+          null, "file1", 1000, 0);
+      Assert.fail("Should have failed");
+    } catch (IllegalArgumentException e){
+      //expected
+    }
+
+    try{
+      new FileCopyGetChunkRequest(requestVersionToUse, 111, "id1",
+          new MockPartitionId(), "", 1000, 0);
+      Assert.fail("Should have failed");
+    } catch (IllegalArgumentException e){
+      //expected
+    }
+
+    try{
+      new FileCopyGetChunkRequest(requestVersionToUse, 111, "id1",
+          new MockPartitionId(), "file1", -1, 0);
+      Assert.fail("Should have failed");
+    } catch (IllegalArgumentException e){
+      //expected
+    }
+
+    try{
+      new FileCopyGetChunkRequest(requestVersionToUse, 111, "id1",
+          new MockPartitionId(), "file1", 1000, -1);
+      Assert.fail("Should have failed");
+    } catch (IllegalArgumentException e){
+      //expected
+    }
+
   }
 
   private void doReplicaMetadataRequestTest(short responseVersionToUse, short requestVersionToUse,