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

Add snowflake id #3500

Merged
merged 26 commits into from
Dec 30, 2021
Merged

Add snowflake id #3500

merged 26 commits into from
Dec 30, 2021

Conversation

jackwener
Copy link
Contributor

@jackwener jackwener commented Dec 20, 2021

What type of PR is this?

  • bug
  • feature

Which issue(s) this PR fixes:

close #3447

What this PR does / why we need it?

Add a kind of unquie id

Special notes for your reviewer, ex. impact of this fix, etc:

Additional context:

Checklist:

  • Documentation affected (If need to modify document, please label it.)
  • Incompatible (If it is incompatile, please describle it and label it.)
  • Need to cherry pick (If need to cherry pick to some branchs, please label the destination version(s).)
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory

Release notes:

Please confirm whether to reflect in release notes and how to describe:

                                                            `

@jackwener jackwener added the wip Solution: work in progress label Dec 20, 2021
@@ -32,6 +32,7 @@ nebula_add_library(
PredicateExpression.cpp
ListComprehensionExpression.cpp
ReduceExpression.cpp
../id/SnowFlake.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add a new object for the ids.

UNUSED(ctx);
SnowFlake generator;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why always new a generator?

int64_t getId();

private:
mutable std::mutex lock_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we still have s lock here?

src/common/id/SnowFlake.cpp Outdated Show resolved Hide resolved
src/common/id/Snowflake.h Show resolved Hide resolved
UNUSED(ctx);
Snowflake generator;
result_ = generator.getId();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add a long term generator for client

src/interface/meta.thrift Show resolved Hide resolved
src/daemons/GraphDaemon.cpp Outdated Show resolved Hide resolved
$<TARGET_OBJECTS:base_obj>
$<TARGET_OBJECTS:snowflake_obj>
LIBRARIES gtest gtest_main
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no new line

@jackwener jackwener added ready for review and removed wip Solution: work in progress labels Dec 27, 2021
@Sophie-Xie Sophie-Xie requested a review from yixinglu December 27, 2021 05:29
@Sophie-Xie Sophie-Xie added the ready-for-testing PR: ready for the CI test label Dec 27, 2021
src/common/id/Snowflake.cpp Outdated Show resolved Hide resolved
src/parser/parser.yy Outdated Show resolved Hide resolved
@@ -8,6 +8,7 @@ nebula_add_test(
$<TARGET_OBJECTS:concurrent_obj>
$<TARGET_OBJECTS:datatypes_obj>
$<TARGET_OBJECTS:expression_obj>
$<TARGET_OBJECTS:snowflake_obj>
Copy link
Contributor

Choose a reason for hiding this comment

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

format

src/common/id/Snowflake.h Outdated Show resolved Hide resolved
src/common/expression/test/CMakeLists.txt Outdated Show resolved Hide resolved
src/common/id/Snowflake.h Outdated Show resolved Hide resolved
src/common/id/Snowflake.h Outdated Show resolved Hide resolved
@jackwener jackwener requested a review from jievince December 29, 2021 08:34
src/interface/common.thrift Outdated Show resolved Hide resolved
src/meta/processors/id/GetWorkerIdProcessor.cpp Outdated Show resolved Hide resolved
src/meta/processors/id/GetWorkerIdProcessor.h Outdated Show resolved Hide resolved
@jackwener jackwener removed the ready-for-testing PR: ready for the CI test label Dec 30, 2021
src/meta/processors/id/GetWorkerIdProcessor.h Outdated Show resolved Hide resolved
src/common/id/Snowflake.h Outdated Show resolved Hide resolved
src/common/id/Snowflake.cpp Show resolved Hide resolved
src/common/id/Snowflake.cpp Outdated Show resolved Hide resolved
src/interface/meta.thrift Show resolved Hide resolved
src/meta/processors/id/GetWorkerIdProcessor.h Outdated Show resolved Hide resolved
@jackwener jackwener added the ready-for-testing PR: ready for the CI test label Dec 30, 2021
Copy link
Contributor

@CPWstatic CPWstatic left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@jievince jievince left a comment

Choose a reason for hiding this comment

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

Good job!

@yixinglu yixinglu merged commit 8676eec into vesoft-inc:master Dec 30, 2021
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Mar 21, 2022
* snowflake id

* fix bug

* fix review

* remove redundant

* tmp

* tmp

* finish

* fix

* fix

* fix header

* fix header

* fix format

* fix test

* fix review

* fix bench

* add 64

* fix review

* fix review

* fix viarable

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>

Co-authored-by: jakevin <30525741+jackwener@users.noreply.github.com>
Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snowflake id
5 participants