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

fix: fix store all wal options #3149

Merged

Conversation

WenyXu
Copy link
Member

@WenyXu WenyXu commented Jan 11, 2024

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

Before we store all region wal options into table metadata or somewhere, we must store all region wal options.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

Refer to a related PR or issue link (optional)

#2700

@WenyXu WenyXu added O-chaos Found by chaos tests docs-not-required This change does not impact docs. O-end-to-end Found by E2E tests labels Jan 11, 2024
@WenyXu WenyXu force-pushed the fix/store-all-filter-wal-options branch from 199952c to 0aa8c8e Compare January 11, 2024 17:51
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (8ec1e42) 85.41% compared to head (e9b2aea) 84.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3149      +/-   ##
==========================================
- Coverage   85.41%   84.92%   -0.50%     
==========================================
  Files         823      823              
  Lines      134797   134798       +1     
==========================================
- Hits       115138   114473     -665     
- Misses      19659    20325     +666     

Copy link
Collaborator

@MichaelScofield MichaelScofield left a comment

Choose a reason for hiding this comment

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

I'm approving this PR to not block the release of v0.6. However, I don't think put all regions' wal options into DatanodeTableValue is a good idea. We need to redesign the table metadata to adapt the overlooked requirement of storing the infomation of regions.

@niebayes
Copy link
Contributor

Seems depends on this #2816

@WenyXu
Copy link
Member Author

WenyXu commented Jan 12, 2024

Seems depends on this #2816

Yes, but it works now. Maybe we can refactor the DatanodeTable and not store the redundant values in the future.

Copy link
Collaborator

@fengjiachun fengjiachun left a comment

Choose a reason for hiding this comment

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

LGTM

@fengjiachun fengjiachun added this pull request to the merge queue Jan 12, 2024
Merged via the queue into GreptimeTeam:main with commit bf88b3b Jan 12, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs. O-chaos Found by chaos tests O-end-to-end Found by E2E tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants