fix: propagate kvstore multi get exception properly #955
Conversation
hzding621
force-pushed
the
haozhen--mussel-partial-failure
branch
from
9cca538 to
4f69380
Compare
I remembered this is already the current behavior right? Did we see any exception case? |
|
@yuli-han this is the expected behavior, but some scenario is not handled correctly previously and is addressed in the PR |
| @@ -670,7 +670,7 @@ class FetcherBase(kvStore: KVStore, | |||
| if (debug || Math.random() < 0.001) { | |||
| logger.error(s"Failed to fetch $groupByRequest", ex) | |||
| } | |||
| Map(groupByRequest.name + "_exception" -> ex.traceString) | |||
| Map(prefix + "_exception" -> ex.traceString) | |||
There was a problem hiding this comment.
Does prefix have the group by's request name?
There was a problem hiding this comment.
yes, prefix here actually stores joinPart.fullPrefix. It's better to use joinPart.fullPrefix than groupByRequest.name because fullPrefix also contains joinPart.prefix.
There are edge cases where two join_part may share the same underlying GroupBy, but different prefix in join part, e.g. prefix = host vs guest, but both user-level GroupBy
| Failure(buildException(e)) | ||
| case Success(resp) => | ||
| if (resp.values.isFailure) { | ||
| Failure(buildException(resp.values.failed.get)) |
There was a problem hiding this comment.
oh so the failure was nested within the response value😑
Good catch.
This reverts commit b45d1c2.
…965) This reverts commit b45d1c2. Co-authored-by: Haozhen Ding <[email protected]>
Summary
Fix
KVStore.getStringsuch that the cases whereKVStore.multiGetreturns aFailurecan be properly handled.Why / Goal
Fetcher should handle join parts failures and return partial results. When KVStore throws exceptions for a certain GroupBy, the exception should be caught and swallowed as an exception string the responseMap instead of failing the whole request.
Test Plan
Checklist
Reviewers
@airbnb/airbnb-chronon-maintainers @pengyu-hou @yuli-han