-
Notifications
You must be signed in to change notification settings - Fork 639
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 surgery clear-page
command
#389
Conversation
p.SetOverflow(0) | ||
if err := guts_cli.WritePage(cmd.dstPath, buf); err != nil { | ||
fmt.Errorf("WritePage failed: %w", err) | ||
} |
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.
Why not make it make it another command in surgeon
?
I imagine it exactly as a place to land this types of 'operations' and CLI should be free of deep business logic assumptions.
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.
Do you mean command surgery write-page
?
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 we should put the logic within this file:
So have there a method:
func ClearPage(path string, pgid pgId) error {...}
and call it from the CLI rather than move the WritePage method into guts_cli.
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.
Partially makes sense to me.
Added ClearPage
in surgeon package. But I still think it makes more sense to get both ReadPage
and WritePage
in package guts_cli
.
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.
Both ReadPage
and WritePage
are more low-level, and it'd better to put them in guts_cli
. While CopyPage
and ClearPage
is relatively high-level as compared to ReadPage & WritePage, so it's OK to put them in surgeon package. .
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'm not proud of ReadPage & WritePage implementation to make them part of shared library.
They are very inefficient in generic usage scenario - i.e. they shouldn't be used to read/write multiple pages. For this reason (and their simplicity) I though about them as private (yes: should be lower case) part of surgeon - not something for a wider consumption in other use-cases.
Second idea is that some day guts_cli.go and page.go should get merged in a single file instead of 2 sibling implementations of the same data model. Thus I would prefer avoid further divergence.
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.
Second idea is that some day guts_cli.go and page.go should get merged in a single file instead of 2 sibling implementations of the same data model. Thus I would prefer avoid further divergence.
Thanks @ptabor . This point totally makes sense to me. We need to refactor the design/implementation, of course in separate PRs.
They are very inefficient in generic usage scenario - i.e. they shouldn't be used to read/write multiple pages. For this reason (and their simplicity) I though about them as private (yes: should be lower case) part of surgeon - not something for a wider consumption in other use-cases.
Yes, I am thinking the same thing, I mean the behavior of reading/writing single or multiple pages. It should be fine for now because the implementation of ReadPage
and WritePage
is Symmetrical. Both of them supporting operating multiple pages.
I'm not proud of ReadPage & WritePage implementation to make them part of shared library.
The implementation of gut_cli
package has too much overlap with the existing bbolt
package, we really need to merge them. It's the next next step we can do.
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.
If the page.overflow > 0
, then it's dangerous to write only one of the pages. If we really need to do this, it definitely should be internal functions/methods. Let's investigate/do this separately.
Signed-off-by: Benjamin Wang <[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.
(don't want to block this pr)
p.SetOverflow(0) | ||
if err := guts_cli.WritePage(cmd.dstPath, buf); err != nil { | ||
fmt.Errorf("WritePage failed: %w", err) | ||
} |
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'm not proud of ReadPage & WritePage implementation to make them part of shared library.
They are very inefficient in generic usage scenario - i.e. they shouldn't be used to read/write multiple pages. For this reason (and their simplicity) I though about them as private (yes: should be lower case) part of surgeon - not something for a wider consumption in other use-cases.
Second idea is that some day guts_cli.go and page.go should get merged in a single file instead of 2 sibling implementations of the same data model. Thus I would prefer avoid further divergence.
My next is to add one more surgery command as I mentioned in #370 (comment). It should be on top of this PR. Afterwards, I might begin to think about #389 (comment) |
Merging for now, let's continue to address all the reminding tasks separately. Thanks again. |
Linked to #370
Signed-off-by: Benjamin Wang [email protected]