-
Notifications
You must be signed in to change notification settings - Fork 59
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
Adjust component for Human readable Bytes #552
Conversation
Changes AnalysisCommit SHA: 8e7c18d API ChangesSummary
ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10722709973/artifacts/1897111192 API Coverage
|
expand_wildcards none with all Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
…nent Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
Spec Test Coverage Analysis
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in CHANGELOG, thanks!
Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
I have a fix for the flaky routing test in #556 |
StorageSize: | ||
type: string | ||
pattern: '\d+(\.\d+)?(b|kb|k|mb|m|gb|g|tb|t|pb|p)' | ||
StorageType: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This naming does not make sense to me. The name would imply this is types of storage (cold, warm, hdd, etc). When in actuality this is byte units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you prefer? My suggestion would be BytesUnit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's reasonable as we do have other ...Unit
enums.
@@ -686,7 +686,10 @@ components: | |||
$ref: '#/components/schemas/NodeId' | |||
NodeId: | |||
type: string | |||
Bytes: | |||
StorageSize: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to my comment on StorageType
, this doesn't seem accurate to me, not all things measured in bytes are storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you prefer? My suggestion would be BytesHuman
or BytesString
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel BytesString
could be mis-interpreted as some mechanism for encoding raw-bytes as a string (ie. a hex/base64 string). Which also kind of applies to Bytes
on it's own too.
Maybe a more verbose ByteCount
and HumanReadableByteCount
would make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original Elastic docs call these "human-formatted", so I think we could use that term, HumanFormattedBytes
, or since these appear a lot in the CAT API, which is "compact and aligned text", we could use BytesText
. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a side note for CAT API.
Due to the bytes
parameter the content differ. Without bytes
parameter it would be printed as "1gb"
and with it as "1024"
. So it would not match the StorageSize
. We would need an extra Type where the unit suffix is optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type can have a anyOf:
with different configurations, one of which is a string with all numbers?
- type: number | ||
- type: string | ||
Bytes: | ||
type: integer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should have had a format: int64
as well
Description
Issues Resolved
Related to #423
Closes #554
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.