-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
🐛 Fix bucket path for destination s3 #11496
Conversation
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, just left a comment on var name
: S3DestinationConstants.DEFAULT_PATH_FORMAT; | ||
final String outputBucketPath = storageOperations.getBucketObjectPath(outputNamespace, streamName, SYNC_DATETIME, customOutputFormat); | ||
final String bucketPath = config.get(BUCKET_PATH_FIELD).asText(); | ||
final String customOutputFormat = String.join("/", |
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.
does customOutputFormat
still make sense as the name here when we're including bucketPath?
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.
you can still customize the end part of the bucket path
it just always starts with the bucketPath now
/publish connector=connectors/destination-s3
|
What
Closes #11482
How
Use s3_bucket_path when constructing the object path
Recommended reading order
x.java
y.python
🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
Updating a connector
/test connector=connectors/<name>
command is passing/publish
command described here