chore: add k8s job resources #6410
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 51df0c4 in 1 minute and 20 seconds
More details
- Looked at
662lines of code in5files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. pkg/query-service/app/infra.go:509
- Draft comment:
Consider providing more specific error messages for better debugging and user feedback. - Reason this comment was not posted:
Confidence changes required:50%
The code for handling job attribute keys and values is consistent with other similar functions. However, the error handling could be improved by providing more specific error messages.
2. pkg/query-service/app/infra.go:525
- Draft comment:
Consider providing more specific error messages for better debugging and user feedback. - Reason this comment was not posted:
Confidence changes required:50%
The code for handling job attribute keys and values is consistent with other similar functions. However, the error handling could be improved by providing more specific error messages.
3. pkg/query-service/app/infra.go:543
- Draft comment:
Consider providing more specific error messages for better debugging and user feedback. - Reason this comment was not posted:
Confidence changes required:50%
The code for handling job list is consistent with other similar functions. However, the error handling could be improved by providing more specific error messages.
4. pkg/query-service/app/inframetrics/common.go:89
- Draft comment:
Consider refactoring to avoid repetition across similar functions likegetParamsForTopJobs,getParamsForTopHosts, etc. - Reason this comment was not posted:
Confidence changes required:50%
The functiongetParamsForTopJobsis consistent with other similar functions. However, the code could be refactored to avoid repetition.
5. pkg/query-service/app/inframetrics/jobs.go:153
- Draft comment:
Consider providing more specific error messages for better debugging and user feedback. - Reason this comment was not posted:
Confidence changes required:50%
TheGetJobAttributeKeysfunction is consistent with other similar functions. However, the error handling could be improved by providing more specific error messages.
6. pkg/query-service/app/inframetrics/jobs.go:178
- Draft comment:
Consider providing more specific error messages for better debugging and user feedback. - Reason this comment was not posted:
Confidence changes required:50%
TheGetJobAttributeValuesfunction is consistent with other similar functions. However, the error handling could be improved by providing more specific error messages.
7. pkg/query-service/app/inframetrics/jobs.go:364
- Draft comment:
Consider providing more specific error messages for better debugging and user feedback. - Reason this comment was not posted:
Confidence changes required:50%
TheGetJobListfunction is consistent with other similar functions. However, the error handling could be improved by providing more specific error messages.
8. pkg/query-service/app/inframetrics/jobs.go:369
- Draft comment:
Consider providing more specific error messages for better debugging and user feedback. - Reason this comment was not posted:
Confidence changes required:50%
TheGetJobListfunction is consistent with other similar functions. However, the error handling could be improved by providing more specific error messages.
9. pkg/query-service/app/http_handler.go:122
- Draft comment:
Do not add non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead. This applies to the addition ofjobsRepoand related methods. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems to be addressing a design concern about the ClickHouseReader interface, but the diff does not show any changes to the ClickHouseReader interface itself. The addition of 'jobsRepo' is related to inframetrics, not ClickHouseReader, so the comment might not be directly relevant to the changes made.
I might be missing the context of how 'jobsRepo' interacts with ClickHouseReader, but based on the diff, there is no direct evidence of a change to ClickHouseReader.
The comment might be preemptively addressing a potential design issue, but without evidence of a direct change to ClickHouseReader, it seems speculative.
The comment does not seem directly relevant to the changes made in the diff, as there is no evidence of a change to ClickHouseReader. The comment should be deleted.
Workflow ID: wflow_YK7sUESKk2ziB763
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
srikanthccv
changed the title
Comment on lines
+34
to
+46
| queryNamesForJobs = map[string][]string{ | ||
| "cpu": {"A"}, | ||
| "cpu_request": {"B", "A"}, | ||
| "cpu_limit": {"C", "A"}, | ||
| "memory": {"D"}, | ||
| "memory_request": {"E", "D"}, | ||
| "memory_limit": {"F", "D"}, | ||
| "restarts": {"G", "A"}, | ||
| "desired_pods": {"H"}, | ||
| "active_pods": {"I"}, | ||
| "failed_pods": {"J"}, | ||
| "successful_pods": {"K"}, | ||
| } |
There was a problem hiding this comment.
nit: rename a, b, c, to some better query names
eg for kafka_fetch_latency I used "latency"
reason: code readability
| }, | ||
| } | ||
|
|
||
| jobQueryNames = []string{"A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K"} |
There was a problem hiding this comment.
if someone also uses similar pattern, the whole codebase would become tricky to decode
A, B, C, ...... X1, X2, ...., AA1 etc namings are not conventional
shivanshuraj1333
approved these changes
Nov 9, 2024
|
I will change the query names for all in another PR. |
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.
Summary
Part of #5373
Important
Adds Kubernetes job metrics handling to the query service with new endpoints, models, and repository for job metrics queries.
APIHandlerinhttp_handler.gowith new endpoints for job attribute keys, values, and list.JobsRepoinjobs.gofor handling job metrics queries.JobListRequest,JobListResponse, andJobListRecordtomodel/infra.go.getParamsForTopJobs()incommon.gofor job metrics aggregation.JobsRepoinjobs.go.infra.goto include job-related API handler functions.This description was created by
for 51df0c4. It will automatically update as commits are pushed.