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 the same name tag and edge in same space #598 #652

Merged
merged 5 commits into from Aug 2, 2019

Conversation

@laura-ding
Copy link
Contributor

commented Jul 18, 2019

close #598

@laura-ding

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

jenkins go

@nebula-community-bot

This comment has been minimized.

Copy link

commented Jul 18, 2019

Unit testing failed.

@nebula-community-bot

This comment has been minimized.

Copy link

commented Jul 18, 2019

Unit testing passed.

@whitewum
Copy link
Contributor

left a comment

Due to the meta client refresh issue, what is the situation if a (conflict) tag add in the refresh period, and then the meta client refresh...
I know this is of very low possibility, but what's the worst case.?

src/graph/ShowExecutor.cpp Show resolved Hide resolved
src/interface/meta.thrift Show resolved Hide resolved
E_BALANCER_RUNNING = -27,
E_CONFLICT = -28,

This comment has been minimized.

Copy link
@whitewum

whitewum Jul 19, 2019

Contributor

why a new errorcode? how about existed? (what about mysql? table vs view)

src/graph/test/SchemaTest.cpp Show resolved Hide resolved
src/graph/ShowExecutor.cpp Show resolved Hide resolved
src/meta/processors/schemaMan/CreateEdgeProcessor.cpp Outdated Show resolved Hide resolved
src/meta/processors/schemaMan/CreateTagProcessor.cpp Outdated Show resolved Hide resolved

@laura-ding laura-ding force-pushed the laura-ding:FixSameName branch from 9f6dcca to 52e29a7 Jul 23, 2019

@nebula-community-bot

This comment has been minimized.

Copy link

commented Jul 23, 2019

Unit testing passed.

@@ -11,6 +11,20 @@ namespace meta {

void CreateEdgeProcessor::process(const cpp2::CreateEdgeReq& req) {
CHECK_SPACE_ID_AND_RETURN(req.get_space_id());
{
// if there is an tag of the same name
folly::SharedMutex::ReadHolder rHolder(LockUtils::tagLock());

This comment has been minimized.

Copy link
@wadeliuyi

wadeliuyi Jul 24, 2019

Contributor

In multithread env, we can not separate this check in two lock, because between read.unlock and write.lock, other thread may insert some same record.

This comment has been minimized.

Copy link
@laura-ding

laura-ding Jul 24, 2019

Author Contributor

Good catch! I have to think about it, I think to use the same lock to do it, but it will handle slower in tag and edge. It is hard to happen when thread A creates tag tagName, thread B create edge tagName at the same timestamp.

This comment has been minimized.

Copy link
@dutor

dutor Jul 24, 2019

Member

@wadeliuyi We might as well leave this potential race condition problem to address in the future.

@laura-ding Better to add a TODO here.

This comment has been minimized.

Copy link
@wadeliuyi

wadeliuyi Jul 24, 2019

Contributor

but "hard to happen" not mean "do not happen", and create tag and edge is not the key path of our program, so performance is not key point?

@@ -11,17 +11,32 @@ namespace meta {

void CreateTagProcessor::process(const cpp2::CreateTagReq& req) {
CHECK_SPACE_ID_AND_RETURN(req.get_space_id());
{
// if there is an edge of the same name
folly::SharedMutex::ReadHolder rHolder(LockUtils::edgeLock());

This comment has been minimized.

Copy link
@wadeliuyi

wadeliuyi Jul 24, 2019

Contributor

ditto

@laura-ding laura-ding force-pushed the laura-ding:FixSameName branch from 52e29a7 to f75f2e4 Jul 26, 2019

@nebula-community-bot

This comment has been minimized.

Copy link

commented Jul 26, 2019

Unit testing passed.

@laura-ding laura-ding force-pushed the laura-ding:FixSameName branch from f75f2e4 to 31b3942 Aug 1, 2019

@nebula-community-bot

This comment has been minimized.

Copy link

commented Aug 1, 2019

Unit testing passed.

@dutor

dutor approved these changes Aug 2, 2019

@dutor dutor merged commit 0e3150c into vesoft-inc:master Aug 2, 2019

1 check passed

UnitTest All tests passed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.