-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add compression to object store #264
Conversation
This commit also cleans up formatting of the spec. Signed-off-by: Tomasz Pietrek <[email protected]>
Signed-off-by: Tomasz Pietrek <[email protected]>
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.
looks some unintended spacing changes crept in
But mainly this doesnt seem to describe what the compression option does? see recent addition to KV adr for sample verbiage etc
adr/ADR-20.md
Outdated
|
||
```go | ||
type ObjectInfo struct { | ||
ObjectMeta | ||
|
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.
not sure these spaces are desired, also below and above
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.
Those spaces were removed in this PR, not added (by markdown linter). I have no idea why you would want them here, but the docs renders the same.
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.
oh god, need more coffee sorry, thought you were adding them lol.
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.
Right I see whats happening, you're removing spaces fine - but I think the entire line should be removed.
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.
Yeah, you're right, removed them entirely.
It was just linter cleaning things up. I didn't want to change more than required for this PR, as I will cleanup the ADR as follow up :).
Signed-off-by: Tomasz Pietrek <[email protected]>
Signed-off-by: Tomasz Pietrek <[email protected]>
@ripienaar the verbiage was compied from KV ADR PR, but I realized it was improved as a follow up :). Improved it a bit. |
Signed-off-by: Tomasz Pietrek <[email protected]>
Signed-off-by: Tomasz Pietrek <[email protected]>
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.
LGTM
This commit also cleans up formatting of the spec.