Bugfix: we address hosts using string(rune(shardID)), not by itoa(shardD) #5952
Conversation
dkrotx
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
taylanisikdemir and
demirkayaender
as code owners
dkrotx
force-pushed
the
fix-shardid-misuse
branch
from
45f0150 to
bdbb15c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
itoa(shardD) I wish it be the other way, but we can't change how it works now since in all the other places we already use rune(shardID). This was not a critical issue - we just returned ShardOwnershipLostError with bad target host in very specific cases. For instance, at the persistence engine error - when we got an error from transaction.
Also some refactoring of existing test: don't duplicate test-name in table-tests and do not mock expectations in SetupTest.
dkrotx
force-pushed
the
fix-shardid-misuse
branch
from
7312cd2 to
d1c9a27
Compare
service/history/handler/handler.go
Outdated
| @@ -2069,7 +2068,7 @@ func (h *handlerImpl) RatelimitUpdate( | |||
| func (h *handlerImpl) convertError(err error) error { | |||
| switch err := err.(type) { | |||
| case *persistence.ShardOwnershipLostError: | |||
| info, err2 := h.GetMembershipResolver().Lookup(service.History, strconv.Itoa(err.ShardID)) | |||
| info, err2 := h.GetMembershipResolver().Lookup(service.History, string(rune(err.ShardID))) | |||
There was a problem hiding this comment.
maybe as a followup PR, but could we have a function that accepts shardID and then hides this logic in 1 place? :)
There was a problem hiding this comment.
That's a good idea!
I'm afraid codecov for new lines won't pass this though :-(
Let me check what's with the coverage of the other multiple lines where Lookup() is used.
There was a problem hiding this comment.
Turns out, not that bad! Had to introduce a new directory to avoid cycle dependencies on go-generate though.
There was a problem hiding this comment.
Good catch and thanks for addressing this. +1 for @3vilhamster 's idea of moving the shardid casting to a common place
so to prevent the same issue in future.
I wish it be the other way, but we can't change how it works now since
in all the other places we already use rune(shardID).
This was not a critical issue - we just returned ShardOwnershipLostError
with bad target host in very specific cases. For instance, at the
persistence engine error - when we got an error from transaction.
Previously we returned wrong host from ShardOwnershipLostError which led cadence-frontend to hit a wrong
host when cadence-history instance lost ownership of the shard during persistance operation.
Everywhere else we use hashring.Lookup(string(rune(shardID))), except for two cases.
Did not check it, looks very much like an error + hard to reproduce (we should be in a middle of persistance Op. for this)
I think it is a clear error now, so let's just fix.
Fixed a bug which sometimes caused a wrong host in ShardOwnershipLostError.