[GitHub] [pulsar] hangc0276 commented on a diff in pull request #15710: [feat] [tiered-storage] Add pure S3 provider for the offloader

[GitBox <gi...@apache.org>]

hangc0276 commented on code in PR #15710:
URL: https://github.com/apache/pulsar/pull/15710#discussion_r880005350


##########
tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/provider/JCloudBlobStoreProvider.java:
##########
@@ -409,14 +428,21 @@ public String getAWSSecretKey() {
         }
     };
 
-    static final CredentialBuilder ALIYUN_OSS_CREDENTIAL_BUILDER = (TieredStorageConfiguration config) -> {
-        String accountName = System.getenv("ALIYUN_OSS_ACCESS_KEY_ID");
-        if (StringUtils.isEmpty(accountName)) {
-            throw new IllegalArgumentException("Couldn't get the aliyun oss access key id.");
+    static final CredentialBuilder S3_CREDENTIAL_BUILDER = (TieredStorageConfiguration config) -> {
+        String accountName = System.getenv("ACCESS_KEY_ID");
+        // For forward compatibility
+        if (StringUtils.isEmpty(accountName.trim())) {

Review Comment:
   It will throw NPE when `accountName` is null. The same below.



##########
tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/provider/TieredStorageConfiguration.java:
##########
@@ -329,6 +329,13 @@ protected Properties getOverrides() {
             overrides.setProperty(S3Constants.PROPERTY_S3_VIRTUAL_HOST_BUCKETS, "false");
         }
 
+        // load more jclouds properties into the overrides
+        System.getProperties().entrySet().stream()

Review Comment:
   `System.getenv()` instead of `System.getProperties()` ?



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org