New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat : linkis-storage support s3 filesystem support write/read launch log and resultSet in s3(#4185) #4435
feat : linkis-storage support s3 filesystem support write/read launch log and resultSet in s3(#4185) #4435
Conversation
return ret; | ||
} else { | ||
logger.warn("File or folder does not exist or file name is garbled(文件或者文件夹不存在或者文件名乱码)"); | ||
throw new StorageWarnException(TO_BE_UNKNOW.getErrorCode(), "文件或者文件夹不存在或者文件名乱码"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should add English
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your advice.I've added the English comments.
} catch (IOException e) { | ||
LOG.warn("get file system failed", e); | ||
} | ||
fs.setUser(user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use proxyUser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, but I don't think need a proxyUser in there. If I am wrong, please let me know.
} | ||
|
||
@Override | ||
public void close() throws IOException {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether there are resources that need to be released when close
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your advice.I checked the code and there were no resources to close.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your advice.I checked the code and there were no resources to close.
ok, thanks!
this.label = label; | ||
} | ||
|
||
public String buildPath(String path) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should Use org.apache.linkis.common.io.FsPath#getSchemaPath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your advice. I use this method because the subdirectories I get through s3client don't have a "/" prefix. I tested this at minIo. The method you mentioned does not meet my needs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your advice. I use this method because the subdirectories I get through s3client don't have a "/" prefix. I tested this at minIo. The method you mentioned does not meet my needs.
ok, thanks!
@@ -83,4 +83,15 @@ object StorageConfiguration { | |||
val FS_CHECKSUM_DISBALE = | |||
CommonVars[java.lang.Boolean]("linkis.fs.hdfs.impl.disable.checksum", false) | |||
|
|||
val S3_ACCESS_KEY = CommonVars("wds.linkis.storage.s3.access.key", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to remove wds prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your advice.I've dropped the prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@sjgllgh code needs to be formatted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
ping @jackxu2011 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What is the purpose of the change
Related issues/PRs
Related issues: #4185
Related pr: #4185
Brief change log
How to use s3 file system
Checklist