feat: add oss storage type in blob sink #58
Conversation
sumitaich1998
changed the title
|
|
||
| public class ObjectStorageService implements BlobStorage { | ||
| private final OSS ossClient; | ||
| private final String bucketName; |
There was a problem hiding this comment.
Should we set the OSSConfig as property? This approach encapsulates all related configuration settings within a single object, making the code cleaner and easier to maintain.
| } | ||
|
|
||
| protected OSS createOSSClient(String endpoint, String accessKeyId, String accessKeySecret) { | ||
| return new OSSClientBuilder().build(endpoint, accessKeyId, accessKeySecret); |
There was a problem hiding this comment.
Should we introduce the option to pass configurable socketTimeout, connectionTimeout, connectionRequestTimeout and requestTimeout? There is also an option to pass configurable retry strategy for the OSSClient
There was a problem hiding this comment.
since we are exposing connection parameters in config, the builder should accommodate the values
| }}; | ||
|
|
||
| BlobStorage storage = BlobStorageFactory.createObjectStorage(BlobStorageType.OSS, config); | ||
| assertTrue(storage instanceof ObjectStorageService); |
There was a problem hiding this comment.
Do we have a way to assert whether the SOME_TYPE_OSS_DIRECTORY_PREFIX is passed correctly? Seems like the assertion is similar with the above test case and this test is redundant if the assertion is all the same.
Same questions with all the positive test cases below, should we also assert the configuration or is it enough just asserting the class of the created object?
| put("OSS_TYPE", "SINK_OBJECT_STORAGE"); | ||
| }}; | ||
| OSSConfig ossConfig = ConfigFactory.create(OSSConfig.class, properties); | ||
| Assert.assertEquals("oss-cn-hangzhou.aliyuncs.com", ossConfig.getOSSEndpoint()); |
There was a problem hiding this comment.
Can add static import for assertion mehods
|
|
||
| - Example value: `1024` | ||
| - Type: `optional` | ||
| - Default value: `1024` |
There was a problem hiding this comment.
is this default value in line with Ali documentation and/or recommendation?
| put("OSS_TYPE", "SINK_OBJECT_STORAGE"); | ||
| }}; | ||
| OSSConfig ossConfig = ConfigFactory.create(OSSConfig.class, properties); | ||
| Assert.assertNull(ossConfig.getOSSDirectoryPrefix()); |
There was a problem hiding this comment.
is the default value for directory prefix empty string or null?
No description provided.