Skip to content
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

improve error handling and signal management #61

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

RadoslavPetkow
Copy link
Contributor

  • Replaced manual signal handling with signal.NotifyContext for cleaner shutdown management.
  • Introduced fatal helper function to reduce redundant error handling and improve readability.
  • Used defer to ensure proper resource cleanup for file and reader operations.
  • Consolidated and streamlined control flow between server mode and job mode.
  • Improved logging messages and ensured Cloud Profiler properly logs initialization failures.

- Replaced manual signal handling with `signal.NotifyContext` for cleaner shutdown management.
- Introduced `fatal` helper function to reduce redundant error handling and improve readability.
- Used `defer` to ensure proper resource cleanup for file and reader operations.
- Consolidated and streamlined control flow between server mode and job mode.
- Improved logging messages and ensured Cloud Profiler properly logs initialization failures.

Signed-off-by: Radoslav Petkov <156192995+RadoslavPetkow@users.noreply.github.com>
Copy link
Member

@kyasbal kyasbal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RadoslavPetkow
First of all, thank you for contributing to our code!
I learned many things from your code, which seems to be very idiomatic in Golang and easier to read. I'm really happy to review this good PR as our first PR contributing the code base.

There is a nit fix and a part that we need to ask you to fix. Let me know if you think there is a better solution rather than my suggestion. We are open to improve.

}

var taskSetRegistrer []inspection.PrepareInspectionServerFunc = make([]inspection.PrepareInspectionServerFunc, 0)
var taskSetRegistrer []inspection.PrepareInspectionServerFunc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var taskSetRegistrer []inspection.PrepareInspectionServerFunc
var taskSetRegistrer []inspection.PrepareInspectionServerFunc = make([]inspection.PrepareInspectionServerFunc, 0)

The reason taskSetRegisterer is initialized with an empty array is actually so that multiple files in the cmd folder have init() and can easily extend the list of tasks.

For example, a user who wants to add functionality for their internal purpose can add a file like private.go to cmd/kubernetes-history-inspector/, add a task to init() to add some set of tasks for their internal purpose, and customize it for internal use by ignoring only private.go with .gitignore.

I know it's impossible to get this purpose just from this code, but this is why we initialized the array with empty array and append tasks on init() instead of assigning a value to the taskRegisterer.

Comment on lines 27 to 28
"sync"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"sync"

nit: This no longer seems to be used anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this is the failure reason of the Github Action

Comment on lines 59 to 63
// Register inspection server preparation functions.
taskSetRegistrer = []inspection.PrepareInspectionServerFunc{
common.PrepareInspectionServer,
gcp.PrepareInspectionServer,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't directly assign the value to the variable because of the reason that I commented few lines before. But thank you for adding comment to make it clear.

Suggested change
// Register inspection server preparation functions.
taskSetRegistrer = []inspection.PrepareInspectionServerFunc{
common.PrepareInspectionServer,
gcp.PrepareInspectionServer,
}
// Register inspection server preparation functions.
taskSetRegistrer = append(taskSetRegistrer, common.PrepareInspectionServer)
taskSetRegistrer = append(taskSetRegistrer, gcp.PrepareInspectionServer)

@kyasbal
Copy link
Member

kyasbal commented Feb 6, 2025

Just for a record. I verified the backend tests are passing on my local computer, even though there is no tests affected by this change.
(Our CI for backend tests are not yet activated for several reason.)

@RadoslavPetkow
Copy link
Contributor Author

Hi ,

Thanks a lot for taking the time to review my PR and for your kind words! I really appreciate the detailed feedback and the insights you shared about the design choices in the codebase.

Feedback Points:

  1. Nit Fix:
    I’ll update the initialization of taskSetRegisterer to use make([]inspection.PrepareInspectionServerFunc, 0) to keep it consistent with the rest of the codebase and align with the intended extensibility via multiple init() functions.
  2. Understanding the Purpose:
    Thanks for explaining why taskSetRegisterer starts as an empty slice. The approach to allow extensions through separate files like private.go for custom use makes a lot of sense, and I completely agree with sticking to it.
  3. Your Suggestion:
    Using append in init() definitely seems like the right way to go. I’ll update the PR accordingly to reflect this.

Thanks again for the thoughtful review and for helping me contribute to this project!

Best,
Radoslav Dimitrov Petkov

RadoslavPetkow

This comment was marked as off-topic.

@kyasbal

This comment was marked as off-topic.

@RadoslavPetkow

This comment was marked as off-topic.

@kyasbal kyasbal assigned kyasbal and unassigned kyasbal Feb 10, 2025
I’ve addressed the requested changes by improving error handling and optimizing redundant logic. The code now avoids panic, ensuring safer execution, and refactors redundant HSL conversions. Let me know if further adjustments are needed. 

Signed-off-by: Radoslav Petkov <156192995+RadoslavPetkow@users.noreply.github.com>
@kyasbal
Copy link
Member

kyasbal commented Feb 14, 2025

@RadoslavPetkow Thank you for keeping working on this. But your last change seems to do something wrong.
I think the cmd/kubernetes-history-inspector/main.go was overwritten with scripts/frontend-codegen/main.go.

@RadoslavPetkow
Copy link
Contributor Author

You’re right! It can be reverted to the last correct version, right?

@kyasbal
Copy link
Member

kyasbal commented Feb 14, 2025

You’re right! It can be reverted to the last correct version, right?

Yes, you just need to checkout the previous revision and commit the change that you actually wanted to and push it.
Because this PR is on your forked repository, you can do force push on your branch.

@kyasbal
Copy link
Member

kyasbal commented Feb 20, 2025

@RadoslavPetkow How should we proceed this?

@RadoslavPetkow

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants