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.
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.
Do you mean command surgery write-page?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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 <wachao@vmware.com>
| 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.
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 wachao@vmware.com