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.

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: support custom storage for every table(initial) #2605

Conversation

NiwakaDev
Copy link
Collaborator

@NiwakaDev NiwakaDev commented Oct 15, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

This PR addresses a part of #919.

Here's an example of using this feature.

create table foo(a string, ts TIMESTAMP TIME INDEX, ~)with(storage='Gcs');

Note that this PR doesn't implement:

  • custom storage for region failover.
  • custom storage for file engine.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

Not close #919 yet.

@NiwakaDev
Copy link
Collaborator Author

I rebased the develop branch, and then rustc says:

error: use of deprecated method `chrono::DateTime::<Tz>::timestamp_nanos`: use `timestamp_nanos_opt()` instead
  --> src/common/time/src/timestamp.rs:66:41
   |
66 |             TimeUnit::Nanosecond => now.timestamp_nanos(),
   |                                         ^^^^^^^^^^^^^^^
   |
   = note: `-D deprecated` implied by `-D warnings`

error: use of deprecated method `chrono::TimeZone::datetime_from_str`: use `DateTime::parse_from_str` instead
   --> src/common/time/src/timestamp.rs:301:29
    |
301 |         if let Ok(ts) = Utc.datetime_from_str(s, "%Y-%m-%d %H:%M:%S%.fZ") {
    |                             ^^^^^^^^^^^^^^^^^

@evenyag
Copy link
Contributor

evenyag commented Oct 16, 2023

@NiwakaDev Maybe caused by a newer opendal version. Our Cargo.lock file uses opendal 0.40.0. Did you try using git merge instead of git rebase?

@NiwakaDev NiwakaDev force-pushed the feat/support_custom_storage_type_for_every_table branch 2 times, most recently from 826a83c to 189eb79 Compare October 16, 2023 12:22
@NiwakaDev NiwakaDev force-pushed the feat/support_custom_storage_type_for_every_table branch from 189eb79 to 527beeb Compare October 16, 2023 23:22
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #2605 (e3bd4ff) into develop (d9e7b89) will decrease coverage by 0.53%.
Report is 4 commits behind head on develop.
The diff coverage is 92.52%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2605      +/-   ##
===========================================
- Coverage    85.33%   84.81%   -0.53%     
===========================================
  Files          737      739       +2     
  Lines       118042   118707     +665     
===========================================
- Hits        100736   100683      -53     
- Misses       17306    18024     +718     

let mut storage_types = Vec::with_capacity(4);
storage_types.push(StorageType::File);
if let Ok(bucket) = env::var("GT_S3_BUCKET") {
if !bucket.is_empty() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GT_S3_BUCKET: ${{ secrets.S3_BUCKET }}
GT_S3_ACCESS_KEY_ID: ${{ secrets.S3_ACCESS_KEY_ID }}
GT_S3_ACCESS_KEY: ${{ secrets.S3_ACCESS_KEY }}
GT_S3_REGION: ${{ secrets.S3_REGION }}
UNITTEST_LOG_DIR: "__unittest_logs"
- name: Codecov upload
uses: codecov/codecov-action@v2

Settings for S3 reside, but it seems like bucket is empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

The S3_BUCKET is only available in this repo's secrets. A PR from a forked repo can't get this secret. So we run the test in the CI after the PR is merged.

@evenyag
Copy link
Contributor

evenyag commented Oct 18, 2023

This feature impacts many modules and contains breaking changes. I'd like to merge it gradually (in several PRs) so we can review and test it carefully. Could you please separate it into smaller PRs? e.g.

  • A PR to add the ObjectStoreManager
  • A PR to support the ObjectStoreManager in mito2 (Now the manager only contains one object store)
  • Support specifying storage in SQL and some tests
  • More tests (To address todos in this PR)

@@ -43,9 +43,10 @@ sync_write = false
[storage]
# The working home directory.
data_home = "/tmp/greptimedb/"
# global_store = "File"
# Storage type.
[[storage.store]]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change. storage.store is a little odd. We'd better find out a new name or revisit the storage config later.

@NiwakaDev
Copy link
Collaborator Author

NiwakaDev commented Oct 18, 2023

This feature impacts many modules and contains breaking changes. I'd like to merge it gradually (in several PRs) so we can review and test it carefully. Could you please separate it into smaller PRs?

Sorry for submitting the large PR. Your point is correct😅. I opened a new PR(#2621), and please review it.

@NiwakaDev NiwakaDev closed this Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom storage type for every table.
2 participants