Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add google as external storage for msq export #16051

Merged
merged 8 commits into from
Apr 5, 2024
Merged

add google as external storage for msq export #16051

merged 8 commits into from
Apr 5, 2024

Conversation

pjain1
Copy link
Member

@pjain1 pjain1 commented Mar 6, 2024

Description

Support for exporting msq results to gcs bucket. This is essentially copying the logic of s3 export for gs, originally done by @adarshsanjeev in this PR - #15689

Release note

Support for exporting MSQ results to a GCS path.

Key changed/added classes in this PR
  • GoogleExportStorageProvider.java
  • GoogleStorageConnectorModule.java

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@adarshsanjeev
Copy link
Contributor

Thanks for the PR! I'll take a look at it shortly.

317brian
317brian requested changes Mar 13, 2024
View reviewed changes
docs/multi-stage-query/reference.md Outdated Show resolved Hide resolved
docs/multi-stage-query/reference.md Outdated Show resolved Hide resolved
docs/multi-stage-query/reference.md Outdated Show resolved Hide resolved
@317brian
Copy link
Contributor

Some minor copyedit nits. The only absolutely must fix one is the incorrect cloud provider being listed in an intro sentence

@pjain1
Copy link
Member Author

pjain1 commented Mar 28, 2024

thanks @317brian added your commits

@pjain1
Copy link
Member Author

pjain1 commented Mar 28, 2024

@adarshsanjeev anything else ?

adarshsanjeev
adarshsanjeev reviewed Apr 4, 2024
View reviewed changes
Copy link
Contributor

@adarshsanjeev adarshsanjeev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. Regarding testing, were these changes tested out on a cluster?

abhishekagarwal87
abhishekagarwal87 reviewed Apr 4, 2024
View reviewed changes
...ogle-extensions/src/main/java/org/apache/druid/storage/google/output/GoogleExportConfig.java
import javax.annotation.Nullable;
import java.util.List;

public class GoogleExportConfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the need to have this class than having these properties directly passed into GoogleExportStorageConnector? It would be nice to have uniformity in various cloud provider implementations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just followed what S3ExportStorageProvider does. I think the reason for separate config is that these configs needs to be injected while creating ExportStorageProvider instance in the sql planning phase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. I see it now.

@pjain1
Copy link
Member Author

pjain1 commented Apr 4, 2024

@adarshsanjeev yes tested on a cluster

@abhishekagarwal87
Copy link
Contributor

@adarshsanjeev - If its looks good to you, can we merge this?

@adarshsanjeev
Copy link
Contributor

Yes, the changes look good to me! Thanks for the PR!

@abhishekagarwal87 abhishekagarwal87 merged commit f55c9e5 into apache:master Apr 5, 2024
85 checks passed
@pjain1 pjain1 deleted the gcs_export_provider branch April 5, 2024 12:41
@adarshsanjeev adarshsanjeev added this to the 30.0.0 milestone May 6, 2024
@adarshsanjeev adarshsanjeev mentioned this pull request May 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhishekagarwal87 abhishekagarwal87 abhishekagarwal87 left review comments

@317brian 317brian 317brian requested changes

@adarshsanjeev adarshsanjeev adarshsanjeev approved these changes

Assignees
No one assigned
Labels
Area - Documentation
Projects
None yet
Milestone
30.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@pjain1 @adarshsanjeev @317brian @abhishekagarwal87