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

Segment auto-increasing id #3550

Merged
merged 32 commits into from
Mar 21, 2022
Merged

Segment auto-increasing id #3550

merged 32 commits into from
Mar 21, 2022

Conversation

jackwener
Copy link
Contributor

@jackwener jackwener commented Dec 23, 2021

What type of PR is this?

  • bug
  • feature
  • enhancement

What does this PR do?

Which issue(s)/PR(s) this PR relates to?

close #3448

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

Additional context:

Checklist:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatible (If it is incompatible, please describe it and add corresponding label.)
  • Need to cherry-pick (If need to cherry-pick to some branches, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

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

Support segment auto-increasing id `

@Sophie-Xie Sophie-Xie added the doc affected PR: improvements or additions to documentation label Dec 23, 2021
@jackwener jackwener added the wip Solution: work in progress label Dec 27, 2021
@Sophie-Xie Sophie-Xie added this to the v3.1.0 milestone Dec 27, 2021
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

Maybe need to consider a method to control the segment size .

src/parser/parser.yy Outdated Show resolved Hide resolved
jievince
jievince previously approved these changes Dec 30, 2021
src/common/id/SegmentId.h Outdated Show resolved Hide resolved
@jievince
Copy link
Contributor

Please ignore my approval, I ordered it wrong

@@ -25,6 +26,8 @@ void UUIDExpression::resetFrom(Decoder& decoder) {

const Value& UUIDExpression::eval(ExpressionContext& ctx) {
UNUSED(ctx);
SegmentId& generator = SegmentId::getInstance(10000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: need to do multiple instance of segment id and snowflake id

@jackwener jackwener added ready for review ready-for-testing PR: ready for the CI test and removed wip Solution: work in progress labels Jan 4, 2022
src/common/id/SegmentId.cpp Outdated Show resolved Hide resolved
src/meta/processors/id/GetSegmentIdProcessor.h Outdated Show resolved Hide resolved
jievince
jievince previously approved these changes Jan 4, 2022

private:
explicit SegmentId(int64_t step) : step_(step) {
segmentStart_ = fetchSegment();
auto xRet = fetchSegment();
Copy link
Contributor

Choose a reason for hiding this comment

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

xRet may not be ok, so you need to move this logic to a init() func.

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.

Ignore my approval...

@jackwener jackwener requested review from yixinglu and jievince January 4, 2022 10:56
src/common/datatypes/test/CMakeLists.txt Outdated Show resolved Hide resolved
src/common/expression/test/CMakeLists.txt Outdated Show resolved Hide resolved
src/common/id/SegmentId.cpp Outdated Show resolved Hide resolved
src/common/id/SegmentId.cpp Outdated Show resolved Hide resolved
tests/tck/features/yield/id.feature Outdated Show resolved Hide resolved
src/common/id/SegmentId.h Outdated Show resolved Hide resolved
@jackwener jackwener force-pushed the segment branch 2 times, most recently from 03d826a to 88afdda Compare January 5, 2022 09:43
@jackwener jackwener removed the doc affected PR: improvements or additions to documentation label Jan 5, 2022
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

It's not a good practice that id generator depends on meta client directly.

jievince
jievince previously approved these changes Jan 7, 2022
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.

LGTM

@jackwener jackwener requested a review from CPWstatic January 10, 2022 02:03
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.

Segment auto-increasing id Auto-generated globally unique vertex id
5 participants