*:Rollback config in store when kv.persist failed #1476
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fix CI and switch to golangci-lint (tikv#1471)
|
Hi contributor, thanks for your PR. This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically. |
1 similar comment
|
Hi contributor, thanks for your PR. This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically. |
siddontang
changed the title
bradyjoestar
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1476 +/- ##
==========================================
+ Coverage 67.87% 68.02% +0.14%
==========================================
Files 159 159
Lines 15781 15843 +62
==========================================
+ Hits 10712 10777 +65
Misses 4104 4104
+ Partials 965 962 -3
Continue to review full report at Codecov.
|
|
/rebuild |
bradyjoestar
changed the title
bradyjoestar
changed the title
rleungx
reviewed
Mar 28, 2019
server/server.go
Outdated
| @@ -622,8 +648,12 @@ func (s *Server) SetNamespaceConfig(name string, cfg NamespaceConfig) error { | |||
| func (s *Server) DeleteNamespaceConfig(name string) error { | |||
| if n, ok := s.scheduleOpt.ns[name]; ok { | |||
| cfg := n.load() | |||
| delete(s.scheduleOpt.ns, name) | |||
| s.scheduleOpt.ns[name].store(&NamespaceConfig{}) | |||
There was a problem hiding this comment.
Delete is not atomic, It may cause DATA RACE error.
I have met it in running test.
There was a problem hiding this comment.
But this way may get error config? all default is zero.
There was a problem hiding this comment.
If ns[name] doesnt exist,it will return a &NamespaceConfig{} , so in a certain sense they are equivalent, I thought
tests/server/region_syncer_test.go
Outdated
| @@ -68,7 +68,7 @@ func (s *serverTestSuite) TestRegionSyncer(c *C) { | |||
| c.Assert(err, IsNil) | |||
| } | |||
| // ensure flush to region kv | |||
| time.Sleep(3 * time.Second) | |||
| time.Sleep(5 * time.Second) | |||
There was a problem hiding this comment.
Once I have met the following logs in jenkins
obtain value: 91
expect value:110
Maybe the time isnt enough? I guess. 😀 :
There was a problem hiding this comment.
I doubt that this change can solve this problem actually. /cc @nolouch
There was a problem hiding this comment.
I'm not sure, maybe relate to the ci environment.
There was a problem hiding this comment.
restore to 3s.
server/server.go
Outdated
| @@ -536,6 +536,11 @@ func (s *Server) SetScheduleConfig(cfg ScheduleConfig) error { | |||
| old := s.scheduleOpt.load() | |||
| s.scheduleOpt.store(&cfg) | |||
| if err := s.scheduleOpt.persist(s.kv); err != nil { | |||
| s.scheduleOpt.store(old) | |||
| log.Error("schedule config updated failed", | |||
There was a problem hiding this comment.
How about changing the error message to failed to update xxx?
@bradyjoestar , here also has a race problem, we need let |
|
/rebuild |
|
Same error will meet |
|
cc @disksing |
disksing
reviewed
May 21, 2019
server/cluster_test.go
Outdated
| @@ -16,6 +16,7 @@ package server | |||
| import ( | |||
| "context" | |||
| "fmt" | |||
| "github.com/pkg/errors" | |||
There was a problem hiding this comment.
Seems not in good order. We recommend to put it with other 3rd part packages.
disksing
reviewed
May 21, 2019
server/option.go
Outdated
| } | ||
| o.ns.Range(f) |
There was a problem hiding this comment.
em, this part is exactly the same with the code block in persist. How about extract a function?
There was a problem hiding this comment.
Sure!:ok_hand:
disksing
reviewed
May 21, 2019
server/server.go
Outdated
| } | ||
| s.scheduleOpt.ns.Range(f) |
There was a problem hiding this comment.
The same pattern comes again :)
|
The rest LGTM. |
disksing
approved these changes
May 21, 2019
|
cc @nolouch |
nolouch
approved these changes
May 22, 2019
nolouch
added a commit
that referenced
this pull request
May 23, 2019
* tiny clean up code (#1526) Signed-off-by: Ryan Leung <rleungx@gmail.com> * config: enable use region storage by default (#1524) * config: default enable use region storage * schedulers: let hot region balance not affect by balance-region-scheduler-limit (#1522) Signed-off-by: nolouch <nolouch@gmail.com> * log: do not add stack to error log (#1532) * *: replace gofail with pingcap/failpoint (#1534) * *: replace gofail with pingcap/failpoint Signed-off-by: disksing <i@disksing.com> * fix typo Signed-off-by: disksing <i@disksing.com> * fix typo Signed-off-by: disksing <i@disksing.com> * Update server/cluster_test.go Co-Authored-By: Lonng <chris@lonng.org> * Update server/tso.go Co-Authored-By: Lonng <chris@lonng.org> * Update server/tso.go Co-Authored-By: Lonng <chris@lonng.org> * check enable failpoint result Signed-off-by: disksing <i@disksing.com> * Update server/cluster.go Co-Authored-By: Lonng <chris@lonng.org> * server: set timeout for MoveLeader (#1533) * server: set timeout for MoveLeader Signed-off-by: disksing <i@disksing.com> * client, server: add ScanRegions gRPC protocol support (#1535) * client, server: support ScanRegions gRPC protocol Signed-off-by: disksing <i@disksing.com> * schedule: actively push operator (#1536) * schedule: actively push operator Signed-off-by: nolouch <nolouch@gmail.com> * *: update some dead links (#1543) * update links Signed-off-by: Ryan Leung <rleungx@gmail.com> * Add windows build script (#1540) Signed-off-by: Ana Hobden <operator@hoverbear.org> * fix operator timeout metrics (#1541) Signed-off-by: Ryan Leung <rleungx@gmail.com> * *:Rollback config in store when kv.persist failed (#1476) * tests: independent region sync test (#1545) * tests: independent syncer region test Signed-off-by: nolouch <nolouch@gmail.com> * schedule: operator limit for stores (#1474) * add store limit for scheduling Signed-off-by: Ryan Leung <rleungx@gmail.com> * statistic: add the statistic of flow (#1548) * store_statistic: add the statistic of flow Signed-off-by: nolouch <nolouch@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
#1475
What is changed and how it works?
rollback the store when persist config failed
Check List
Tests