[FileCopy based bootstrap][FileCopyHandler] Add FCHandler #3035
Conversation
…lecopy-handler
…lecopy-handler
…lecopy-handler
…n/ambry into dw-filecopy-handler
…lecopy-handler
…lecopy-handler
| * Timeout duration for file chunk operations in minutes. | ||
| * After this duration, incomplete chunk operations are considered failed. | ||
| */ | ||
| public static final String FILECOPY_FILE_CHUNK_TIMEOUT_IN_MINUTES = "filecopy.file.chunk.timeout.in.minutes"; |
There was a problem hiding this comment.
Let's Keep the other config. It has better naming conventions for file. Matches with File Copy module names.
There was a problem hiding this comment.
Fine either way. I matched the file content. Its same.
Seemed 'FileCopyBasedReplicationConfig' is bit of an overkill for this name vs FileCopyConfig.
There was a problem hiding this comment.
Update; So AmbryLI has FileCopyBasedReplicationConfig committed and its the deployed code. When I tried testing a jar from this current PR's diff, AmbryLI's server bootstrap failed because it was expecting that class.
With lesser amount of changes required, I am reverting to using this class and delete the other dup FileCopyConfig class.
...-file-transfer/src/main/java/com/github/ambry/filetransfer/handler/StoreFileCopyHandler.java
Show resolved
Hide resolved
| /** | ||
| * Configuration for FileCopyHandler | ||
| */ | ||
| public class FileCopyHandlerConfig { |
There was a problem hiding this comment.
Should we merge it to File Copy Based Replication Config. Creating separate config classes. will require too many config object to be sent from upstream classes.
| @Nonnull ConnectionPool connectionPool, | ||
| @Nonnull StoreManager storeManager, | ||
| @Nonnull ClusterMap clusterMap, | ||
| @Nonnull FileCopyHandlerConfig config) { |
There was a problem hiding this comment.
Can we use File Copy config instead.
| * Shutdown the file copy handler. Perform clean up steps in case of a graceful shutdown. | ||
| */ | ||
| void shutdown() { | ||
| connectionPool.shutdown(); |
There was a problem hiding this comment.
Are we planning to create a seperate connection pool for File Copy. We should not close it in File Copy Handler if this is a shared connection pool.
| * @throws IOException | ||
| */ | ||
| @Override | ||
| public void copy(@Nonnull FileCopyInfo fileCopyInfo) throws IOException { |
There was a problem hiding this comment.
Should we break FileCopyInfo into multiple objects.
- To identify replica and other information. That can be sent to file copy handler and it need not have the chunk information.
- And other object used for getting chunks. which will be used inside the FileCopy Handler.
There was a problem hiding this comment.
We don't need to mutate FileCopyinfo on every call.
| @@ -37,7 +38,7 @@ public class FileCopyThread implements Runnable { | |||
| public void run() { | |||
| try { | |||
| //TODO add required params for File copy handler | |||
| fileCopyHandler.copy(); | |||
| fileCopyHandler.copy(null); | |||
| fileCopyStatusListener.onFileCopySuccess(); | |||
| } catch (Exception e) { | |||
| fileCopyStatusListener.onFileCopyFailure(e); | |||
There was a problem hiding this comment.
We need Peerfinder and retries in case of failures.
| public void testGetFileCopyGetMetaDataResponseExpectConnectionTimeout() throws Exception { | ||
| // Arrange: Mock ConnectionPoolTimeoutException | ||
| when(handler.getOperationRetryHandler().executeWithRetry(any(), eq(GetMetadataWorkflow.GET_METADATA_OPERATION_NAME))) | ||
| .thenThrow(new ConnectionPoolTimeoutException("Timeout")); |
There was a problem hiding this comment.
Do we need to override the default timeout set for Connection Pool for File Copy or make it configurable?
| * @throws Exception exception | ||
| */ | ||
| void copy() throws Exception; | ||
| void copy(@Nonnull FileCopyInfo fileCopyInfo) |
There was a problem hiding this comment.
why do we need FileCopyInfo?
| /** | ||
| * The hostname of the target node | ||
| */ | ||
| private final String hostName; |
Summary
BUG=AMBRY-13117
Adds support for FileCopyHandler that orchestrates GetMetadata and GetChunk Apis given a Replica and a target peer node.
Testing Done
Test suite WIP