Skip to content
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

cmd: migrate 'surgery copy-page' command to cobra style comamnd #481

Merged
merged 1 commit into from
May 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 52 additions & 4 deletions cmd/bbolt/command_surgery_cobra.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ var (
)

var (
surgeryTargetDBFilePath string
surgeryPageId uint64
surgeryStartElementIdx int
surgeryEndElementIdx int
surgeryTargetDBFilePath string
surgeryPageId uint64
surgeryStartElementIdx int
surgeryEndElementIdx int
surgerySourcePageId uint64
surgeryDestinationPageId uint64
)

func newSurgeryCobraCommand() *cobra.Command {
Expand All @@ -31,6 +33,7 @@ func newSurgeryCobraCommand() *cobra.Command {
}

surgeryCmd.AddCommand(newSurgeryRevertMetaPageCommand())
surgeryCmd.AddCommand(newSurgeryCopyPageCommand())
surgeryCmd.AddCommand(newSurgeryClearPageElementsCommand())
surgeryCmd.AddCommand(newSurgeryFreelistCommand())

Expand Down Expand Up @@ -78,6 +81,51 @@ func surgeryRevertMetaPageFunc(cmd *cobra.Command, args []string) error {
return nil
}

func newSurgeryCopyPageCommand() *cobra.Command {
copyPageCmd := &cobra.Command{
Use: "copy-page <bbolt-file> [options]",
Short: "Copy page from the source page Id to the destination page Id",
Args: func(cmd *cobra.Command, args []string) error {
if len(args) == 0 {
return errors.New("db file path not provided")
}
if len(args) > 1 {
return errors.New("too many arguments")
}
return nil
},
RunE: surgeryCopyPageFunc,
}

copyPageCmd.Flags().StringVar(&surgeryTargetDBFilePath, "output", "", "path to the target db file")
copyPageCmd.Flags().Uint64VarP(&surgerySourcePageId, "from-page", "", 0, "source page Id")
copyPageCmd.Flags().Uint64VarP(&surgeryDestinationPageId, "to-page", "", 0, "destination page Id")

return copyPageCmd
}

func surgeryCopyPageFunc(cmd *cobra.Command, args []string) error {
srcDBPath := args[0]

if surgerySourcePageId == surgeryDestinationPageId {
return fmt.Errorf("'--from-page' and '--to-page' have the same value: %d", surgerySourcePageId)
}

if err := common.CopyFile(srcDBPath, surgeryTargetDBFilePath); err != nil {
return fmt.Errorf("[copy-page] copy file failed: %w", err)
}

if err := surgeon.CopyPage(surgeryTargetDBFilePath, common.Pgid(surgerySourcePageId), common.Pgid(surgeryDestinationPageId)); err != nil {
return fmt.Errorf("copy-page command failed: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shell we warn if the freelist is persisted and recommend dropping it ?

This might cause unreachable pages being reachable again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! Added,

	fmt.Fprintf(os.Stdout, "WARNING: the free list might have changed.\n")
	fmt.Fprintf(os.Stdout, "Please consider executing `./bbolt surgery abandon-freelist ...`\n")

Copy link
Member Author

Choose a reason for hiding this comment

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

thx @ptabor

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for quick fix.
Ideally it should be a helper function that checks whether freelist is enabled before printing this warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, let me do a minor refactoring in a followup PR,

meta, err := readMetaPage(srcDBPath)
if err != nil {
return err
}
if meta.Freelist() != common.PgidNoFreelist {

Comment on lines +84 to +120
Copy link
Member

Choose a reason for hiding this comment

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

Let's use Kubernetes flag pattern, it's much more extensible and compositable.

Suggested change
func newSurgeryCopyPageCommand() *cobra.Command {
copyPageCmd := &cobra.Command{
Use: "copy-page <bbolt-file> [options]",
Short: "Copy page from the source page Id to the destination page Id",
Args: func(cmd *cobra.Command, args []string) error {
if len(args) == 0 {
return errors.New("db file path not provided")
}
if len(args) > 1 {
return errors.New("too many arguments")
}
return nil
},
RunE: surgeryCopyPageFunc,
}
copyPageCmd.Flags().StringVar(&surgeryTargetDBFilePath, "output", "", "path to the target db file")
copyPageCmd.Flags().Uint64VarP(&surgerySourcePageId, "from-page", "", 0, "source page Id")
copyPageCmd.Flags().Uint64VarP(&surgeryDestinationPageId, "to-page", "", 0, "destination page Id")
return copyPageCmd
}
func surgeryCopyPageFunc(cmd *cobra.Command, args []string) error {
srcDBPath := args[0]
if surgerySourcePageId == surgeryDestinationPageId {
return fmt.Errorf("'--from-page' and '--to-page' have the same value: %d", surgerySourcePageId)
}
if err := common.CopyFile(srcDBPath, surgeryTargetDBFilePath); err != nil {
return fmt.Errorf("[copy-page] copy file failed: %w", err)
}
if err := surgeon.CopyPage(surgeryTargetDBFilePath, common.Pgid(surgerySourcePageId), common.Pgid(surgeryDestinationPageId)); err != nil {
return fmt.Errorf("copy-page command failed: %w", err)
}
type surgeryOptions struct {
surgeryTargetDBFilePath string
surgeryPageId uint64
surgeryStartElementIdx int
surgeryEndElementIdx int
surgerySourcePageId uint64
surgeryDestinationPageId uint64
}
func defaultSurgeryOptions() surgeryOptions {
return surgeryOptions{}
}
func (o *surgeryOptions) Validate() error {
if o.surgerySourcePageId == o.surgeryDestinationPageId {
return fmt.Errorf("'--from-page' and '--to-page' have the same value: %d", surgerySourcePageId)
}
}
func (o *Options) AddFlags(fs flag.FlagSet) {
fs.StringVar(&o.surgeryTargetDBFilePath, "output", o.surgeryTargetDBFilePath, "path to the target db file")
fs.Uint64Var(&o.surgerySourcePageId, "from-page", o.surgerySourcePageId, "source page Id")
fs.Uint64Var(&o.surgeryDestinationPageId, "to-page", o.surgeryDestinationPageId, "destination page Id")
}
func newSurgeryCopyPageCommand() *cobra.Command {
cfg := defaultSurgeryOptions()
cmd := &cobra.Command{
Use: "copy-page <bbolt-file> [options]",
Short: "Copy page from the source page Id to the destination page Id",
Args: func(c *cobra.Command, args []string) error {
if len(args) == 0 {
return errors.New("db file path not provided")
}
if len(args) > 1 {
return errors.New("too many arguments")
}
return nil
},
RunE: func(cmd *cobra.Command, args []string) error {
err := o.Validate()
if err != nil {
return err
}
return surgeryCopyPageFunc(args[0], o)
},
}
o.AddFlags(cmd.Flags())
return cmd
}
func surgeryCopyPageFunc(srcDBPath string, o *surgeryOptions) error {
if err := common.CopyFile(srcDBPath, surgeryTargetDBFilePath); err != nil {
return fmt.Errorf("[copy-page] copy file failed: %w", err)
}
if err := surgeon.CopyPage(surgeryTargetDBFilePath, common.Pgid(surgerySourcePageId), common.Pgid(surgeryDestinationPageId)); err != nil {
return fmt.Errorf("copy-page command failed: %w", err)
}
fmt.Fprintf(os.Stdout, "The page %d was successfully copied to page %d\n", surgerySourcePageId, surgeryDestinationPageId)
return nil
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The struct surgeryOptions makes sense, but not the (*surgeryOptions) Validate(), because different surgery commands have different validation logic.

Let me apply the struct surgeryOptions together @ptabor 's comment above in a separate PR. Regarding to K8s flag pattern, I do not see any essential difference or obvious benefits. But please feel free to raise a separate issue or draft PR to discuss it. thx

Copy link
Member Author

Choose a reason for hiding this comment

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

After second look, your suggestion indeed is better, it removes all the global variables. Resolved this comment in #483 . thx


fmt.Fprintf(os.Stdout, "WARNING: the free list might have changed.\n")
fmt.Fprintf(os.Stdout, "Please consider executing `./bbolt surgery abandon-freelist ...`\n")

fmt.Fprintf(os.Stdout, "The page %d was successfully copied to page %d\n", surgerySourcePageId, surgeryDestinationPageId)
return nil
}

func newSurgeryClearPageElementsCommand() *cobra.Command {
clearElementCmd := &cobra.Command{
Use: "clear-page-elements <bbolt-file> [options]",
Expand Down
59 changes: 59 additions & 0 deletions cmd/bbolt/command_surgery_cobra_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,44 @@ func TestSurgery_RevertMetaPage(t *testing.T) {
assert.Equal(t, pageDataWithoutPageId(nonActiveSrcBuf), pageDataWithoutPageId(dstBuf1))
}

func TestSurgery_CopyPage(t *testing.T) {
pageSize := 4096
db := btesting.MustCreateDBWithOption(t, &bolt.Options{PageSize: pageSize})
srcPath := db.Path()

// Insert some sample data
t.Log("Insert some sample data")
err := db.Fill([]byte("data"), 1, 20,
func(tx int, k int) []byte { return []byte(fmt.Sprintf("%04d", k)) },
func(tx int, k int) []byte { return make([]byte, 10) },
)
require.NoError(t, err)

defer requireDBNoChange(t, dbData(t, srcPath), srcPath)

// copy page 3 to page 2
t.Log("copy page 3 to page 2")
rootCmd := main.NewRootCommand()
output := filepath.Join(t.TempDir(), "dstdb")
rootCmd.SetArgs([]string{
"surgery", "copy-page", srcPath,
"--output", output,
"--from-page", "3",
"--to-page", "2",
})
err = rootCmd.Execute()
require.NoError(t, err)

// The page 2 should have exactly the same data as page 3.
t.Log("Verify result")
srcPageId3Data := readPage(t, srcPath, 3, pageSize)
dstPageId3Data := readPage(t, output, 3, pageSize)
dstPageId2Data := readPage(t, output, 2, pageSize)

assert.Equal(t, srcPageId3Data, dstPageId3Data)
assert.Equal(t, pageDataWithoutPageId(srcPageId3Data), pageDataWithoutPageId(dstPageId2Data))
}

func TestSurgery_ClearPageElements_Without_Overflow(t *testing.T) {
testCases := []struct {
name string
Expand Down Expand Up @@ -578,3 +616,24 @@ func readMetaPage(t *testing.T, path string) *common.Meta {
require.NoError(t, err)
return common.LoadPageMeta(buf)
}

func readPage(t *testing.T, path string, pageId int, pageSize int) []byte {
dbFile, err := os.Open(path)
require.NoError(t, err)
defer dbFile.Close()

fi, err := dbFile.Stat()
require.NoError(t, err)
require.GreaterOrEqual(t, fi.Size(), int64((pageId+1)*pageSize))

buf := make([]byte, pageSize)
byteRead, err := dbFile.ReadAt(buf, int64(pageId*pageSize))
require.NoError(t, err)
require.Equal(t, pageSize, byteRead)

return buf
}

func pageDataWithoutPageId(buf []byte) []byte {
return buf[8:]
}
64 changes: 0 additions & 64 deletions cmd/bbolt/surgery_commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ func (cmd *surgeryCommand) Run(args ...string) error {
case "help":
fmt.Fprintln(cmd.Stderr, cmd.Usage())
return ErrUsage
case "copy-page":
return newCopyPageCommand(cmd).Run(args[1:]...)
case "clear-page":
return newClearPageCommand(cmd).Run(args[1:]...)
default:
Expand Down Expand Up @@ -81,73 +79,11 @@ Usage:
The commands are:
help print this screen
clear-page clear all elements at the given pageId
copy-page copy page from source pageId to target pageId
revert-meta-page revert the meta page change made by the last transaction

Use "bbolt surgery [command] -h" for more information about a command.
`, "\n")
}

// copyPageCommand represents the "surgery copy-page" command execution.
type copyPageCommand struct {
*surgeryCommand
}

// newCopyPageCommand returns a copyPageCommand.
func newCopyPageCommand(m *surgeryCommand) *copyPageCommand {
c := &copyPageCommand{}
c.surgeryCommand = m
return c
}

// Run executes the command.
func (cmd *copyPageCommand) Run(args ...string) error {
// Parse flags.
fs := flag.NewFlagSet("", flag.ContinueOnError)
help := fs.Bool("h", false, "")
if err := fs.Parse(args); err != nil {
return err
} else if *help {
fmt.Fprintln(cmd.Stderr, cmd.Usage())
return ErrUsage
}

if err := cmd.parsePathsAndCopyFile(fs); err != nil {
return fmt.Errorf("copyPageCommand failed to parse paths and copy file: %w", err)
}

// Read page id.
srcPageId, err := strconv.ParseUint(fs.Arg(2), 10, 64)
if err != nil {
return err
}
dstPageId, err := strconv.ParseUint(fs.Arg(3), 10, 64)
if err != nil {
return err
}

// copy the page
if err := surgeon.CopyPage(cmd.dstPath, common.Pgid(srcPageId), common.Pgid(dstPageId)); err != nil {
return fmt.Errorf("copyPageCommand failed: %w", err)
}

fmt.Fprintf(cmd.Stdout, "The page %d was copied to page %d\n", srcPageId, dstPageId)
return nil
}

// Usage returns the help message.
func (cmd *copyPageCommand) Usage() string {
return strings.TrimLeft(`
usage: bolt surgery copy-page SRC DST srcPageId dstPageid

CopyPage copies the database file at SRC to a newly created database
file at DST. Afterwards, it copies the page at srcPageId to the page
at dstPageId in DST.

The original database is left untouched.
`, "\n")
}

// clearPageCommand represents the "surgery clear-page" command execution.
type clearPageCommand struct {
*surgeryCommand
Expand Down
54 changes: 0 additions & 54 deletions cmd/bbolt/surgery_commands_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package main_test

import (
"fmt"
"os"
"path/filepath"
"testing"

Expand All @@ -14,38 +13,6 @@ import (
"go.etcd.io/bbolt/internal/common"
)

func TestSurgery_CopyPage(t *testing.T) {
pageSize := 4096
db := btesting.MustCreateDBWithOption(t, &bolt.Options{PageSize: pageSize})
srcPath := db.Path()

// Insert some sample data
t.Log("Insert some sample data")
err := db.Fill([]byte("data"), 1, 20,
func(tx int, k int) []byte { return []byte(fmt.Sprintf("%04d", k)) },
func(tx int, k int) []byte { return make([]byte, 10) },
)
require.NoError(t, err)

defer requireDBNoChange(t, dbData(t, srcPath), srcPath)

// copy page 3 to page 2
t.Log("copy page 3 to page 2")
dstPath := filepath.Join(t.TempDir(), "dstdb")
m := NewMain()
err = m.Run("surgery", "copy-page", srcPath, dstPath, "3", "2")
require.NoError(t, err)

// The page 2 should have exactly the same data as page 3.
t.Log("Verify result")
srcPageId3Data := readPage(t, srcPath, 3, pageSize)
dstPageId3Data := readPage(t, dstPath, 3, pageSize)
dstPageId2Data := readPage(t, dstPath, 2, pageSize)

assert.Equal(t, srcPageId3Data, dstPageId3Data)
assert.Equal(t, pageDataWithoutPageId(srcPageId3Data), pageDataWithoutPageId(dstPageId2Data))
}

// TODO(ahrtr): add test case below for `surgery clear-page` command:
// 1. The page is a branch page. All its children should become free pages.
func TestSurgery_ClearPage(t *testing.T) {
Expand Down Expand Up @@ -78,24 +45,3 @@ func TestSurgery_ClearPage(t *testing.T) {
assert.Equal(t, uint16(0), p.Count())
assert.Equal(t, uint32(0), p.Overflow())
}

func readPage(t *testing.T, path string, pageId int, pageSize int) []byte {
dbFile, err := os.Open(path)
require.NoError(t, err)
defer dbFile.Close()

fi, err := dbFile.Stat()
require.NoError(t, err)
require.GreaterOrEqual(t, fi.Size(), int64((pageId+1)*pageSize))

buf := make([]byte, pageSize)
byteRead, err := dbFile.ReadAt(buf, int64(pageId*pageSize))
require.NoError(t, err)
require.Equal(t, pageSize, byteRead)

return buf
}

func pageDataWithoutPageId(buf []byte) []byte {
return buf[8:]
}