[GitHub] [doris] morningman commented on a diff in pull request #14405: [improvement] (fix) Add check validity when create storage policy.

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

morningman commented on code in PR #14405:
URL: https://github.com/apache/doris/pull/14405#discussion_r1039759359


##########
fe/fe-core/src/main/java/org/apache/doris/catalog/S3Resource.java:
##########
@@ -121,12 +126,51 @@ protected void setProperties(Map<String, String> properties) throws DdlException
         checkRequiredProperty(S3_ACCESS_KEY);
         checkRequiredProperty(S3_SECRET_KEY);
         checkRequiredProperty(S3_BUCKET);
+
+        // default need check resource conf valid, so need fix ut and regression case
+        boolean needCheck = !properties.containsKey(S3_VALIDITY_CHECK)
+                || Boolean.parseBoolean(properties.get(S3_VALIDITY_CHECK));
+        LOG.debug("s3 info need check validity : {}", needCheck);
+        if (needCheck) {
+            boolean available = pingS3();
+            if (!available) {
+                throw new DdlException("S3 can't use, please check your properties");
+            }
+        }
+
         // optional
         checkOptionalProperty(S3_MAX_CONNECTIONS, DEFAULT_S3_MAX_CONNECTIONS);
         checkOptionalProperty(S3_REQUEST_TIMEOUT_MS, DEFAULT_S3_REQUEST_TIMEOUT_MS);
         checkOptionalProperty(S3_CONNECTION_TIMEOUT_MS, DEFAULT_S3_CONNECTION_TIMEOUT_MS);
     }
 
+    private boolean pingS3() {
+        String bucket = "s3://" + properties.getOrDefault(S3_BUCKET, "") + "/";
+        Map<String, String> propertiesPing = new HashMap<>();
+        propertiesPing.put("AWS_ACCESS_KEY", properties.getOrDefault(S3_ACCESS_KEY, ""));
+        propertiesPing.put("AWS_SECRET_KEY", properties.getOrDefault(S3_SECRET_KEY, ""));
+        propertiesPing.put("AWS_ENDPOINT", "http://" + properties.getOrDefault(S3_ENDPOINT, ""));
+        propertiesPing.put("AWS_REGION", properties.getOrDefault(S3_REGION, ""));
+        propertiesPing.put(S3Storage.USE_PATH_STYLE, "false");
+        S3Storage storage = new S3Storage(propertiesPing);
+
+        String testFile = bucket + properties.getOrDefault(S3_ROOT_PATH, "") + "/test-object-valid.txt";
+        String content = "doris will be better";
+        Status status = storage.directUpload(content, testFile);
+        if (status != Status.OK) {
+            LOG.warn("ping update file status: {}, properties: {}", status, propertiesPing);
+            return false;
+        }
+
+        Status delete = storage.delete(testFile);

Review Comment:
   wrap with `try .. finally`? To make sure the file will be deleted finally.



##########
fe/fe-core/src/main/java/org/apache/doris/backup/S3Storage.java:
##########
@@ -245,6 +245,9 @@ public Status directUpload(String content, String remoteFile) {
         } catch (UserException ue) {
             LOG.error("connect to s3 failed: ", ue);
             return new Status(Status.ErrCode.COMMON_ERROR, "connect to s3 failed: " + ue.getMessage());
+        } catch (Exception e) {
+            LOG.error("connect to s3 failed: ", e);

Review Comment:
   Use `warn`, need to modify above all LOG.error to LOG.warn



-- 
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@doris.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org