-
Notifications
You must be signed in to change notification settings - Fork 725
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
*:Rollback config in store when kv.persist failed #1476
Conversation
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. |
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete is not atomic, It may cause DATA RACE
error.
I have met it in running test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this way may get error config? all default is zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ns[name]
doesnt exist,it will return a &NamespaceConfig{}
, so in a certain sense they are equivalent, I thought
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt that this change can solve this problem actually. /cc @nolouch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, maybe relate to the ci environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about changing the error message to failed to update xxx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. 👌
@bradyjoestar , here also has a race problem, we need let |
/rebuild |
Same error will meet |
cc @disksing |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems not in good order. We recommend to put it with other 3rd part packages.
server/option.go
Outdated
} | ||
o.ns.Range(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!:ok_hand:
server/server.go
Outdated
} | ||
s.scheduleOpt.ns.Range(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same pattern comes again :)
The rest LGTM. |
cc @nolouch |
* 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>
What problem does this PR solve?
#1475
What is changed and how it works?
rollback the store when persist config failed
Check List
Tests