fix(navigation/tablet-errors): fix showing errors for replicated tables#1342
Open
ainthero wants to merge 1 commit intoytsaurus:mainfrom
Open
fix(navigation/tablet-errors): fix showing errors for replicated tables#1342ainthero wants to merge 1 commit intoytsaurus:mainfrom
ainthero wants to merge 1 commit intoytsaurus:mainfrom
Conversation
…umming @replicas.*.error_count + @tablet_error_count
ainthero
requested review from
KostyaAvtushko,
SimbiozizV,
evtaranov and
ma-efremoff
as code owners
Reviewer's GuideUpdates the tablet error count flow by revising the batch API request to retrieve the full replicas map, applying type-safe aggregation of per-replica error_count with fallback handling, and combining it with the table-level error counter. Sequence diagram for updated tablet error count aggregationsequenceDiagram
participant Dispatch
participant Toaster
participant ytApiV3Id
participant UpdateTabletErrorsCount
Dispatch->>Toaster: wrapApiPromiseByToaster(ytApiV3Id.executeBatch(...))
Toaster->>ytApiV3Id: executeBatch with requests:\n- get replicas map\n- get tablet_error_count
ytApiV3Id-->>Toaster: [replicas, tabletErrorCount]
Toaster->>Dispatch: then()
Dispatch->>UpdateTabletErrorsCount: updateTabletErrrosCount(replicasErrors + tableErrors, path)
Class diagram for updated error count aggregation logicclassDiagram
class LoadTabletErrorOptions {
path: string
saveCancelTokenSource: any
}
class TabletErrorsThunkAction
class ytApiV3Id {
+executeBatch(parameters)
}
class updateTabletErrrosCount {
+updateTabletErrrosCount(count, path)
}
LoadTabletErrorOptions <.. TabletErrorsThunkAction
TabletErrorsThunkAction --> ytApiV3Id: uses
TabletErrorsThunkAction --> updateTabletErrrosCount: dispatches
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Replace the use of
anyfor the API response with a properly typed interface to improve type safety and catch parsing issues at compile time. - Consider extracting the replica error-count summing logic into a standalone helper function to simplify the thunk and make it easier to unit test.
- Rather than swallowing all errors in the catch block, log or surface parsing failures so debugging unexpected replica formats is easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Replace the use of `any` for the API response with a properly typed interface to improve type safety and catch parsing issues at compile time.
- Consider extracting the replica error-count summing logic into a standalone helper function to simplify the thunk and make it easier to unit test.
- Rather than swallowing all errors in the catch block, log or surface parsing failures so debugging unexpected replica formats is easier.
## Individual Comments
### Comment 1
<location> `packages/ui/src/ui/store/actions/navigation/tabs/tablet-errors/tablet-errors-background.ts:147-149` </location>
<code_context>
- dispatch(updateTabletErrrosCount(count, path));
+ ).then(([{output: replicas}, {output: tabletErrorCount}]) => {
+ let replicasErrors = 0;
+ if (replicas && typeof replicas === 'object') {
+ try {
+ replicasErrors = Object.values(replicas as Record<string, any>)
+ .map((r: any) => (typeof r?.error_count === 'number' ? r.error_count : 0))
+ .reduce((a: number, b: number) => a + b, 0);
+ } catch {
+ replicasErrors = 0;
+ }
</code_context>
<issue_to_address>
**suggestion:** Catching all errors without logging may hide underlying issues.
Consider logging the error in the catch block or adding a comment to clarify why silent failure is acceptable, to support future debugging.
```suggestion
} catch (error) {
console.error('Failed to calculate replicasErrors:', error);
replicasErrors = 0;
}
```
</issue_to_address>
### Comment 2
<location> `packages/ui/src/ui/store/actions/navigation/tabs/tablet-errors/tablet-errors-background.ts:151-152` </location>
<code_context>
+ replicasErrors = 0;
+ }
+ }
+ const tableErrors = typeof tabletErrorCount === 'number' ? tabletErrorCount : 0;
+ dispatch(updateTabletErrrosCount(replicasErrors + tableErrors, path));
});
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Tablet error count fallback to 0 may mask unexpected API responses.
Consider logging a warning or error when tabletErrorCount is not a number to help identify unexpected API responses early.
```suggestion
let tableErrors: number;
if (typeof tabletErrorCount === 'number') {
tableErrors = tabletErrorCount;
} else {
console.warn(
`[tablet-errors-background] Unexpected tabletErrorCount value:`,
tabletErrorCount,
'for path:',
path
);
tableErrors = 0;
}
dispatch(updateTabletErrrosCount(replicasErrors + tableErrors, path));
```
</issue_to_address>
### Comment 3
<location> `packages/ui/src/ui/store/actions/navigation/tabs/tablet-errors/tablet-errors-background.ts:152` </location>
<code_context>
+ }
+ }
+ const tableErrors = typeof tabletErrorCount === 'number' ? tabletErrorCount : 0;
+ dispatch(updateTabletErrrosCount(replicasErrors + tableErrors, path));
});
};
</code_context>
<issue_to_address>
**nitpick (typo):** Typo in 'updateTabletErrrosCount' function name.
Please rename the function to 'updateTabletErrorsCount'.
Suggested implementation:
```typescript
dispatch(updateTabletErrorsCount(replicasErrors + tableErrors, path));
```
If the function `updateTabletErrrosCount` is defined or imported elsewhere in this file, you should also rename its definition or import to `updateTabletErrorsCount`. Additionally, update any other usage of the old name in this file or related files to maintain consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+147
to
+149
| } catch { | ||
| replicasErrors = 0; | ||
| } |
There was a problem hiding this comment.
suggestion: Catching all errors without logging may hide underlying issues.
Consider logging the error in the catch block or adding a comment to clarify why silent failure is acceptable, to support future debugging.
Suggested change
| } catch { | |
| replicasErrors = 0; | |
| } | |
| } catch (error) { | |
| console.error('Failed to calculate replicasErrors:', error); | |
| replicasErrors = 0; | |
| } |
| } | ||
| } | ||
| const tableErrors = typeof tabletErrorCount === 'number' ? tabletErrorCount : 0; | ||
| dispatch(updateTabletErrrosCount(replicasErrors + tableErrors, path)); |
There was a problem hiding this comment.
nitpick (typo): Typo in 'updateTabletErrrosCount' function name.
Please rename the function to 'updateTabletErrorsCount'.
Suggested implementation:
dispatch(updateTabletErrorsCount(replicasErrors + tableErrors, path));If the function updateTabletErrrosCount is defined or imported elsewhere in this file, you should also rename its definition or import to updateTabletErrorsCount. Additionally, update any other usage of the old name in this file or related files to maintain consistency.
Collaborator
|
Can you please provide more details? Why do we need the fix? |
Author
In the current version, the "Errors" tab doesn’t appear for replicated tables when there are errors on replicas or tablets, unlike dyntables. You have to go to the "Tablets" tab, find the tablet with an error, open it, and only then you can see the error details. This fix resolves that issue. |
Collaborator
|
Thank you for the details, I will discuss the fix with API team. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by Sourcery
Fix showing errors for replicated tables by updating the API batch request to retrieve the replicas map, summing per-replica error counts, and combining them with the table-level error count.
Bug Fixes:
Enhancements: