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

refactor: allocate table id in the procedure #3271

Merged

Conversation

WenyXu
Copy link
Member

@WenyXu WenyXu commented Jan 31, 2024

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

  1. refactor: replace TableMetadataManager with TableNameManager
  2. refactor: allocate table id in the procedure
  3. feat: add test utils for testing CreateTableProcedure
  4. test: add tests for CreateTableProcedure

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)

#3233

@WenyXu WenyXu marked this pull request as draft January 31, 2024 08:34
@github-actions github-actions bot added docs-not-required This change does not impact docs. Size: M and removed docs-not-required This change does not impact docs. labels Jan 31, 2024
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

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

Comparison is base (450dfe3) 85.26% compared to head (3bcb4f6) 84.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3271      +/-   ##
==========================================
- Coverage   85.26%   84.87%   -0.39%     
==========================================
  Files         881      883       +2     
  Lines      143843   144014     +171     
==========================================
- Hits       122641   122226     -415     
- Misses      21202    21788     +586     

@github-actions github-actions bot added docs-not-required This change does not impact docs. Size: L and removed Size: M labels Feb 6, 2024
@WenyXu WenyXu force-pushed the enhance/create-table-ddl-procedure branch 2 times, most recently from f9cea39 to c3c7a03 Compare February 6, 2024 09:37
@WenyXu WenyXu marked this pull request as ready for review February 6, 2024 09:37
@WenyXu WenyXu force-pushed the enhance/create-table-ddl-procedure branch from c3c7a03 to d52307b Compare February 6, 2024 09:39
@WenyXu WenyXu requested a review from waynexia February 6, 2024 09:41
@WenyXu WenyXu force-pushed the enhance/create-table-ddl-procedure branch from d52307b to 0d58b1f Compare February 6, 2024 09:43
@WenyXu WenyXu requested a review from zhongzc February 6, 2024 09:46
@github-actions github-actions bot added Size: XL and removed Size: L labels Feb 6, 2024
src/common/meta/src/ddl/utils.rs Outdated Show resolved Hide resolved
src/common/procedure/src/procedure.rs Outdated Show resolved Hide resolved
@WenyXu WenyXu requested a review from fengjiachun February 18, 2024 06:15
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

@WenyXu WenyXu requested review from fengys1996 and v0y4g3r February 19, 2024 08:19
src/common/meta/src/ddl/create_table.rs Show resolved Hide resolved
src/common/meta/src/ddl/create_table.rs Outdated Show resolved Hide resolved
src/common/meta/src/ddl/tests/create_table.rs Outdated Show resolved Hide resolved
src/common/procedure/src/procedure.rs Show resolved Hide resolved
@WenyXu WenyXu force-pushed the enhance/create-table-ddl-procedure branch from 949511a to 7cd1c62 Compare February 20, 2024 14:56
@MichaelScofield MichaelScofield added this pull request to the merge queue Feb 21, 2024
Merged via the queue into GreptimeTeam:main with commit 41656c8 Feb 21, 2024
16 checks passed
MichaelScofield pushed a commit to MichaelScofield/greptimedb that referenced this pull request Feb 21, 2024
* refactor: replace TableMetadataManager with TableNameManager

* refactor: allocate table id in the procedure

* refactor: refactor client logical of handling retries

* feat(test_util): add TestCreateTableExprBuilder

* feat(test_util): add MockDatanodeManager

* feat(test_util): add new_ddl_context

* feat(test_util): add build_raw_table_info_from_expr

* feat(test_util): add MockDatanodeManager::new

* feat(procedure): add downcast_output_ref to Status

* test(create_table): add tests for CreateTableProcedure on_prepare

* refactor(ddl): rename handle_operate_region_error to add_peer_context_if_need

* test(create_table): add tests for CreateTableProcedure on_datanode_create_regions

* test(create_table): add tests for CreateTableProcedure on_create_metadata

* refactor(meta): use CreateTableExprBuilder

* feat(create_table): ensure number of partitions is greater than 0

* refactor: rename to add_peer_context_if_needed

* feat: add context for panic

* refactor: simplify the should_retry

* refactor: use Option<&T> instead of &Option<T>

* refactor: move downcast_output_ref under cfg(test)

* chore: fmt toml
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants