Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

feat: add CreateTableProcedure #73

Merged
merged 4 commits into from
Oct 19, 2022
Merged

feat: add CreateTableProcedure #73

merged 4 commits into from
Oct 19, 2022

Conversation

chunshao90
Copy link
Contributor

@chunshao90 chunshao90 commented Oct 14, 2022 •
edited
Loading

Which issue does this PR close?

Closes #

Rationale for this change

Add CreateTableProcedure implementation.

What changes are included in this PR?

  • Modify function in cluster
GetOrCreateTable(ctx context.Context, nodeName string, schemaName string, tableName string) (*Table, error)
->GetOrCreateTable(ctx context.Context, nodeName string, schemaName string, tableName string) (*Table, bool, error)
  • Add GetShardByID function in cluster.Shard.
  • Add CreateTableProcedure implementation.

Are there any user-facing changes?

How does this change test

Add TestCreateTable test.

@chunshao90 chunshao90 marked this pull request as draft October 14, 2022 08:16
@chunshao90 chunshao90 force-pushed the feat-create-table-procedure branch 3 times, most recently from ec7ff61 to 6f7f5e5 Compare October 18, 2022 06:39
@chunshao90 chunshao90 marked this pull request as ready for review October 18, 2022 06:43
ZuLiangWang
ZuLiangWang reviewed Oct 18, 2022
View reviewed changes
server/cluster/manager.go Outdated Show resolved Hide resolved
server/cluster/cluster.go Outdated Show resolved Hide resolved
ShiKaiWi
ShiKaiWi reviewed Oct 19, 2022
View reviewed changes
Copy link
Member

@ShiKaiWi ShiKaiWi left a comment

Choose a reason for hiding this comment

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

I find a problem with the current procedure implement, that is to say, there is no mechanism to notify the caller (procedure creator) with the procedure running result. And for this reason, create/drop table seems no way to work properly.

server/cluster/cluster.go Show resolved Hide resolved
server/cluster/cluster.go Show resolved Hide resolved
server/cluster/manager.go Show resolved Hide resolved
server/coordinator/procedure/create_table.go Show resolved Hide resolved
server/coordinator/procedure/create_table.go Show resolved Hide resolved
server/coordinator/procedure/scatter_test.go Outdated Show resolved Hide resolved
ShiKaiWi
ShiKaiWi approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@ShiKaiWi ShiKaiWi left a comment

Choose a reason for hiding this comment

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

LGTM

@chunshao90 chunshao90 merged commit 5c95406 into apache:main Oct 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ZuLiangWang ZuLiangWang ZuLiangWang left review comments

@ShiKaiWi ShiKaiWi ShiKaiWi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@chunshao90 @ShiKaiWi @ZuLiangWang