chenboat commented on code in PR #9710:
URL: https://github.com/apache/pinot/pull/9710#discussion_r1024601467
##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/BaseTableDataManagerTest.java:
##########
@@ -614,6 +622,61 @@ public void testDownloadAndDecrypt()
}
}
+ // case 2: if the attempt to download from deep storage exceeds, invoke
downloadFromPeers.
+ @Test
+ public void testDownloadAndDecryptCase2() throws Exception {
Review Comment:
NIT: instead of Case2, be more specific in the test name: it is about
downloading from peers.
##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/BaseTableDataManagerTest.java:
##########
@@ -614,6 +622,61 @@ public void testDownloadAndDecrypt()
}
}
+ // case 2: if the attempt to download from deep storage exceeds, invoke
downloadFromPeers.
+ @Test
+ public void testDownloadAndDecryptCase2() throws Exception {
+ File tempInput = new File(TEMP_DIR, "tmp.txt");
+ FileUtils.write(tempInput, "this is from somewhere remote");
+
+ SegmentZKMetadata zkmd = mock(SegmentZKMetadata.class);
+ String backupCopyURI = "file://" + tempInput.getAbsolutePath();
+ when(zkmd.getDownloadUrl()).thenReturn(backupCopyURI);
+
+ TableDataManagerConfig config = createDefaultTableDataManagerConfig();
+ when(config.getTablePeerDownloadScheme()).thenReturn("http");
+ BaseTableDataManager tmgr = createSpyOfflineTableManager(config);
+ File tempRootDir = tmgr.getTmpSegmentDataDir("test-download-decrypt-peer");
+
+ // As the case 2 description says, we need to mock the static method
fetchAndDecryptSegmentToLocal to
+ // throw the AttemptExceed exception; Due to the constrain that mockito
static cannot do argument matching,
+ // e.g., any(), we have to pass exact argument value when mocking
fetchAndDecryptSegmentToLocal.
+ // However, the first argument File is internally created, which cannot be
mocked.
+ // Luckily, the File class's equal method only compares the path. Thus, we
can create a file with identical path
+ // and use it to mock the fetchAndDecryptSegmentToLocal
+ File destFile = new File(tempRootDir, "seg01" +
TarGzCompressionUtils.TAR_GZ_FILE_EXTENSION);
+ doNothing().when(tmgr).downloadFromPeersWithoutStreaming("seg01", zkmd,
destFile);
+ try (MockedStatic<SegmentFetcherFactory> mockSegFactory =
mockStatic(SegmentFetcherFactory.class)) {
+ mockSegFactory.when(() ->
SegmentFetcherFactory.fetchAndDecryptSegmentToLocal(backupCopyURI, destFile,
null))
+ .thenThrow(new AttemptsExceededException("fake attempt exceeds
exception"));
+ tmgr.downloadAndDecrypt("seg01", zkmd, tempRootDir);
+ }
+ verify(tmgr, times(1)).downloadFromPeersWithoutStreaming("seg01", zkmd,
destFile);
+ }
+
+ // happy case: down
+ @Test
+ public void testdownloadFromPeersWithoutStreaming() throws Exception {
Review Comment:
Nit: testDownload...
##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/BaseTableDataManagerTest.java:
##########
@@ -614,6 +622,61 @@ public void testDownloadAndDecrypt()
}
}
+ // case 2: if the attempt to download from deep storage exceeds, invoke
downloadFromPeers.
+ @Test
+ public void testDownloadAndDecryptCase2() throws Exception {
+ File tempInput = new File(TEMP_DIR, "tmp.txt");
+ FileUtils.write(tempInput, "this is from somewhere remote");
+
+ SegmentZKMetadata zkmd = mock(SegmentZKMetadata.class);
+ String backupCopyURI = "file://" + tempInput.getAbsolutePath();
+ when(zkmd.getDownloadUrl()).thenReturn(backupCopyURI);
+
+ TableDataManagerConfig config = createDefaultTableDataManagerConfig();
+ when(config.getTablePeerDownloadScheme()).thenReturn("http");
+ BaseTableDataManager tmgr = createSpyOfflineTableManager(config);
+ File tempRootDir = tmgr.getTmpSegmentDataDir("test-download-decrypt-peer");
+
+ // As the case 2 description says, we need to mock the static method
fetchAndDecryptSegmentToLocal to
+ // throw the AttemptExceed exception; Due to the constrain that mockito
static cannot do argument matching,
+ // e.g., any(), we have to pass exact argument value when mocking
fetchAndDecryptSegmentToLocal.
+ // However, the first argument File is internally created, which cannot be
mocked.
+ // Luckily, the File class's equal method only compares the path. Thus, we
can create a file with identical path
+ // and use it to mock the fetchAndDecryptSegmentToLocal
+ File destFile = new File(tempRootDir, "seg01" +
TarGzCompressionUtils.TAR_GZ_FILE_EXTENSION);
+ doNothing().when(tmgr).downloadFromPeersWithoutStreaming("seg01", zkmd,
destFile);
+ try (MockedStatic<SegmentFetcherFactory> mockSegFactory =
mockStatic(SegmentFetcherFactory.class)) {
+ mockSegFactory.when(() ->
SegmentFetcherFactory.fetchAndDecryptSegmentToLocal(backupCopyURI, destFile,
null))
+ .thenThrow(new AttemptsExceededException("fake attempt exceeds
exception"));
+ tmgr.downloadAndDecrypt("seg01", zkmd, tempRootDir);
+ }
+ verify(tmgr, times(1)).downloadFromPeersWithoutStreaming("seg01", zkmd,
destFile);
+ }
+
+ // happy case: down
Review Comment:
download
##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/BaseTableDataManagerTest.java:
##########
@@ -614,6 +622,61 @@ public void testDownloadAndDecrypt()
}
}
+ // case 2: if the attempt to download from deep storage exceeds, invoke
downloadFromPeers.
+ @Test
+ public void testDownloadAndDecryptCase2() throws Exception {
+ File tempInput = new File(TEMP_DIR, "tmp.txt");
+ FileUtils.write(tempInput, "this is from somewhere remote");
+
+ SegmentZKMetadata zkmd = mock(SegmentZKMetadata.class);
+ String backupCopyURI = "file://" + tempInput.getAbsolutePath();
+ when(zkmd.getDownloadUrl()).thenReturn(backupCopyURI);
+
+ TableDataManagerConfig config = createDefaultTableDataManagerConfig();
+ when(config.getTablePeerDownloadScheme()).thenReturn("http");
+ BaseTableDataManager tmgr = createSpyOfflineTableManager(config);
+ File tempRootDir = tmgr.getTmpSegmentDataDir("test-download-decrypt-peer");
+
+ // As the case 2 description says, we need to mock the static method
fetchAndDecryptSegmentToLocal to
+ // throw the AttemptExceed exception; Due to the constrain that mockito
static cannot do argument matching,
+ // e.g., any(), we have to pass exact argument value when mocking
fetchAndDecryptSegmentToLocal.
+ // However, the first argument File is internally created, which cannot be
mocked.
+ // Luckily, the File class's equal method only compares the path. Thus, we
can create a file with identical path
+ // and use it to mock the fetchAndDecryptSegmentToLocal
+ File destFile = new File(tempRootDir, "seg01" +
TarGzCompressionUtils.TAR_GZ_FILE_EXTENSION);
+ doNothing().when(tmgr).downloadFromPeersWithoutStreaming("seg01", zkmd,
destFile);
+ try (MockedStatic<SegmentFetcherFactory> mockSegFactory =
mockStatic(SegmentFetcherFactory.class)) {
+ mockSegFactory.when(() ->
SegmentFetcherFactory.fetchAndDecryptSegmentToLocal(backupCopyURI, destFile,
null))
+ .thenThrow(new AttemptsExceededException("fake attempt exceeds
exception"));
+ tmgr.downloadAndDecrypt("seg01", zkmd, tempRootDir);
+ }
+ verify(tmgr, times(1)).downloadFromPeersWithoutStreaming("seg01", zkmd,
destFile);
+ }
+
+ // happy case: down
+ @Test
+ public void testdownloadFromPeersWithoutStreaming() throws Exception {
+ File tempInput = new File(TEMP_DIR, "tmp.txt");
Review Comment:
A lot of test codes are shared between the two new test methods. Can we
refactor them?
--
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]
