Skip to content

Review suggestions#35

Open
thaJeztah wants to merge 6 commits intodqminh:psifrom
thaJeztah:psi_suggestions
Open

Review suggestions#35
thaJeztah wants to merge 6 commits intodqminh:psifrom
thaJeztah:psi_suggestions

Conversation

@thaJeztah
Copy link
Copy Markdown

Review suggestions for opencontainers#3358

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Comment thread types/events.go
type (
PSIData = cgroups.PSIData
PSIStats = cgroups.PSIStats
)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

my impression was that we tried to make types separate from cgroups as much as possible, hence the disconnection and duplication here.

It's reasonable to say that we do not care about the separation (i.e. the types package is not really useful), in that case i would prefer to do the refactoring separately.

CpuUsage CpuUsage `json:"cpu_usage,omitempty"`
ThrottlingData ThrottlingData `json:"throttling_data,omitempty"`
PSI PSIStats `json:"psi,omitempty"`
PSI *PSIStats `json:"psi,omitempty"`
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it's preferable to be consistent with the rest, and do the refactoring later.

Comment thread types/events.go
Usage CpuUsage `json:"usage,omitempty"`
Throttling Throttling `json:"throttling,omitempty"`
PSI PSIStats `json:"psi,omitempty"`
PSI *PSIStats `json:"psi,omitempty"`
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it's preferable to be consistent with the rest, and do the refactoring later.

Comment thread types/events.go
KernelTCP MemoryEntry `json:"kernelTCP,omitempty"`
Raw map[string]uint64 `json:"raw,omitempty"`
PSI PSIStats `json:"psi,omitempty"`
PSI *PSIStats `json:"psi,omitempty"`
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it's preferable to be consistent with the rest, and do the refactoring later.

}
if err := sc.Err(); err != nil {
err = sc.Err()
if err != nil {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

do we need this change ?

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