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

domain: change schema validator data struct to queue #4578

Merged
merged 15 commits into from Sep 25, 2017

Conversation

Projects
None yet
5 participants
@tiancaiamao
Contributor

tiancaiamao commented Sep 19, 2017

schema change history is organized using (array + map), where map serves as array data index.
I think it's more suitable to use a queue, and always keep the latest few changes.

@zimulala @coocood

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Sep 19, 2017

Member

What's the benefit of this PR?

Member

shenli commented Sep 19, 2017

What's the benefit of this PR?

Show outdated Hide outdated domain/domain.go
Show outdated Hide outdated domain/schema_validator.go
Show outdated Hide outdated domain/schema_validator.go
@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 20, 2017

Member

@tiancaiamao
I think the basic slice can achieve the same goal.

// enqueue
deltaItemInfos = append(deltalItemInfos, info)
if len(deltaItemInfo) > maxLength {
    deltaItemInfos = deltaItemInfos[1:]
}

It's more straightforward.

Member

coocood commented Sep 20, 2017

@tiancaiamao
I think the basic slice can achieve the same goal.

// enqueue
deltaItemInfos = append(deltalItemInfos, info)
if len(deltaItemInfo) > maxLength {
    deltaItemInfos = deltaItemInfos[1:]
}

It's more straightforward.

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 20, 2017

Contributor

The code is really elegant! I'll take your advise. @coocood

Contributor

tiancaiamao commented Sep 20, 2017

The code is really elegant! I'll take your advise. @coocood

tiancaiamao added some commits Sep 21, 2017

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 21, 2017

Contributor

@shenli
In the old implementation, schema change history organized as a fix sized array, when the array is full, we'll reset the history.
If a transaction's schema version not in the history, we say it's changed, but that maybe a false positive because we just reset the history.
Organize the schema change history as a queue doesn't suffer the problem, that data struct would always record the latest changes.

Contributor

tiancaiamao commented Sep 21, 2017

@shenli
In the old implementation, schema change history organized as a fix sized array, when the array is full, we'll reset the history.
If a transaction's schema version not in the history, we say it's changed, but that maybe a false positive because we just reset the history.
Organize the schema change history as a queue doesn't suffer the problem, that data struct would always record the latest changes.

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 21, 2017

Member

How about defining a function named findNewerDeltas which returns a slice of items, then iterate the newer deltas to find related tables.

Member

coocood commented Sep 21, 2017

How about defining a function named findNewerDeltas which returns a slice of items, then iterate the newer deltas to find related tables.

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 21, 2017

Contributor

findNewerDeltas is a good idea.

Contributor

tiancaiamao commented Sep 21, 2017

findNewerDeltas is a good idea.

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 21, 2017

Member

LGTM

Member

coocood commented Sep 21, 2017

LGTM

@coocood coocood added the status/LGT1 label Sep 23, 2017

@@ -80,17 +76,14 @@ func (s *schemaValidator) Stop() {
defer s.mux.Unlock()
s.isStarted = false
s.latestSchemaVer = 0
s.detalItemInfos = nil
s.itemSchemaVers = nil
s.deltaSchemaInfos = make([]deltaSchemaInfo, 0, maxNumberOfDiffsToLoad)

This comment has been minimized.

@zimulala

zimulala Sep 23, 2017

Member

Because of the implement of enqueue, maybe we can set the capability value as maxNumberOfDiffsToLoad +1

@zimulala

zimulala Sep 23, 2017

Member

Because of the implement of enqueue, maybe we can set the capability value as maxNumberOfDiffsToLoad +1

@zimulala

This comment has been minimized.

Show comment
Hide comment
@zimulala

zimulala Sep 23, 2017

Member

@tiancaiamao
I don't understand that you said: "when the array is full, we'll reset the history". I don't see this logic in the old implementation. It only removes the too old(the latest version - one's version > 100) version information, likewise, the new implementation keeps 100 versions information.

Member

zimulala commented Sep 23, 2017

@tiancaiamao
I don't understand that you said: "when the array is full, we'll reset the history". I don't see this logic in the old implementation. It only removes the too old(the latest version - one's version > 100) version information, likewise, the new implementation keeps 100 versions information.

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 23, 2017

Member

@tiancaiamao
I just found there may be an issue that the delta schema info may not be complete latest 100 versions.
Some of the deltas may be missing, then we can make sure no tables changed since a transaction used schema version.

When you Stop then Restart the validator, many schema version may be updated and those versions we do not have in our deltaSchemaInfos.

Another place to check is that when tryLoadSchemaDiffs not ok, it means over 100 versions are missed, in this case, deltaSchemaInfos are invalid, we should clear it.

Member

coocood commented Sep 23, 2017

@tiancaiamao
I just found there may be an issue that the delta schema info may not be complete latest 100 versions.
Some of the deltas may be missing, then we can make sure no tables changed since a transaction used schema version.

When you Stop then Restart the validator, many schema version may be updated and those versions we do not have in our deltaSchemaInfos.

Another place to check is that when tryLoadSchemaDiffs not ok, it means over 100 versions are missed, in this case, deltaSchemaInfos are invalid, we should clear it.

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 25, 2017

Contributor

@zimulala
You're right, I misunderstand that and thought it reset the history.

Contributor

tiancaiamao commented Sep 25, 2017

@zimulala
You're right, I misunderstand that and thought it reset the history.

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 25, 2017

Contributor

/run-all-tests

Contributor

tiancaiamao commented Sep 25, 2017

/run-all-tests

Show outdated Hide outdated domain/domain.go
@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 25, 2017

Member

LGTM

Member

coocood commented Sep 25, 2017

LGTM

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 25, 2017

Contributor

/run-all-tests

Contributor

tiancaiamao commented Sep 25, 2017

/run-all-tests

@zimulala zimulala added status/LGT2 and removed status/LGT1 labels Sep 25, 2017

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 25, 2017

Contributor

/run-unit-test

Contributor

tiancaiamao commented Sep 25, 2017

/run-unit-test

@coocood coocood merged commit 0d19c5d into master Sep 25, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@coocood coocood deleted the tiancaiamao/schema-validator branch Sep 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment