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

chore: cleanup globals in pkg/cgroup #1726

Merged

Conversation

maryamtahhan
Copy link
Collaborator

@maryamtahhan maryamtahhan commented Aug 22, 2024

cleanup globals in pkg/cgroup

Copy link
Contributor

github-actions bot commented Aug 22, 2024

🤖 SeineSailor

Here is a concise summary of the pull request changes:

Summary: This pull request refactors the cgroup package to eliminate global variables, introducing a new cache struct to store container information. The changes improve code organization and reduce coupling between components.

Key Modifications:

  1. Global variable removal: Replaced global variables with cache fields, reducing the risk of unintended side effects and improving code maintainability.
  2. Cache management functions: Added InitCache(), SetConfig(), and newCache() functions to manage the cache instance, making it easier to initialize and configure the cache.
  3. Internal refactoring: Refactored internal functions and variables to use the new cache instance, improving code organization and reducing complexity.

Impact on Codebase:

  • Improved code maintainability and organization
  • Reduced coupling between components
  • Simplified cache management

Observations and Suggestions:

  • The changes are mostly internal and do not affect the external interface or behavior of the code.
  • It would be beneficial to add unit tests to ensure the new cache implementation works correctly.
  • Consider documenting the new cache struct and its management functions to facilitate understanding and usage.

Overall, this pull request improves the codebase's maintainability and organization, making it easier to understand and modify the cgroup package.

@maryamtahhan maryamtahhan marked this pull request as ready for review August 26, 2024 08:18
@maryamtahhan maryamtahhan marked this pull request as draft August 26, 2024 08:21
@maryamtahhan maryamtahhan force-pushed the cgroup-cleanup-globals branch from 8d69828 to 6661609 Compare August 26, 2024 12:27
@maryamtahhan maryamtahhan marked this pull request as ready for review August 26, 2024 12:35
@maryamtahhan maryamtahhan force-pushed the cgroup-cleanup-globals branch from 6661609 to c562464 Compare August 28, 2024 12:27
@rootfs rootfs requested review from sthaha and sunya-ch August 29, 2024 15:40
@rootfs
Copy link
Contributor

rootfs commented Aug 29, 2024

@sthaha @sunya-ch can you take a look? thanks

Copy link
Collaborator

@sunya-ch sunya-ch left a comment

Choose a reason for hiding this comment

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

refactoring by moving all variable to cache struct is fine to me. Just leave some comments.

pkg/cgroup/resolve_container.go Outdated Show resolved Hide resolved
pkg/cgroup/resolve_container.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@sthaha sthaha left a comment

Choose a reason for hiding this comment

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

requesting changes to consider

  • use of init
  • remove ensureCacheInitialised
  • let cache handle when Mutex should be locked / unlocked

@maryamtahhan maryamtahhan force-pushed the cgroup-cleanup-globals branch 2 times, most recently from 9f1978b to ae99a51 Compare August 30, 2024 13:28
@maryamtahhan
Copy link
Collaborator Author

maryamtahhan commented Sep 2, 2024

requesting changes to consider

  • use of init
  • remove ensureCacheInitialised
  • let cache handle when Mutex should be locked / unlocked

Hi @sthaha
I incorporated the suggested changes. Please let me know if there's anything else.

Thanks

@maryamtahhan
Copy link
Collaborator Author

@sthaha sorry to poke again, can you have a look at the updates?

Thanks again

@sthaha
Copy link
Collaborator

sthaha commented Sep 18, 2024

@sthaha sorry to poke again, can you have a look at the updates?

Thanks again

Sorry, this completely slipped my radar :( ..

pkg/cgroup/resolve_container.go Outdated Show resolved Hide resolved
pkg/cgroup/resolve_container.go Outdated Show resolved Hide resolved
pkg/cgroup/resolve_container.go Outdated Show resolved Hide resolved
pkg/cgroup/resolve_container.go Outdated Show resolved Hide resolved
pkg/cgroup/resolve_container.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@sthaha sthaha left a comment

Choose a reason for hiding this comment

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

Thanks @maryamtahhan

@sthaha sthaha merged commit ffab0ff into sustainable-computing-io:main Sep 18, 2024
20 checks passed
@maryamtahhan maryamtahhan deleted the cgroup-cleanup-globals branch September 19, 2024 08:29
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.

4 participants