From df8e427ce86b36c0bf2cf730a070e3cd222b32e0 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Mon, 5 Aug 2024 16:06:35 +0300 Subject: [PATCH 01/29] Add simple mocking and test for skip files for doRestoreAgent --- helper/helper.go | 13 ++++- helper/helper_test.go | 115 +++++++++++++++++++++++++++++++++++++++ helper/restore_helper.go | 36 +++++++++--- 3 files changed, 154 insertions(+), 10 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index 56564342b..ea8c1e9bd 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -212,7 +212,18 @@ func preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCo } } -func getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { +type Helper interface { + getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) + createPipe(pipe string) error +} + +type HelperImpl struct{} + +func (h HelperImpl) createPipe(pipe string) error { + return createPipe(pipe) +} + +func (h HelperImpl) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { oidStr, err := operating.System.ReadFile(oidFileName) if err != nil { logError(fmt.Sprintf("Error encountered reading oid batch list from file: %v", err)) diff --git a/helper/helper_test.go b/helper/helper_test.go index b0bbf7160..d7f716022 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -1,12 +1,87 @@ package helper import ( + "bufio" + "os" + "github.com/greenplum-db/gpbackup/utils" + "golang.org/x/sys/unix" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + + "github.com/greenplum-db/gpbackup/toc" + "github.com/pkg/errors" ) +type test_step struct { + getRestorePipeWriter_arg_expect string + getRestorePipeWriter_result bool + check_skip_file_arg_tableoid int + check_skip_file_result bool +} + +type MockHelperImpl struct { + step_no int + expected_steps []test_step +} + +func (h MockHelperImpl) getCurStep() test_step { + return h.expected_steps[h.step_no] +} + +var test_expected_scenario = []test_step{ + {}, // placeholder as steps start from 1 + {"mock_100_2", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called + {"mock_200_3", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called + {"mock_200_4", false, 200, false}, // Can not open pipe for table 200, check_skip_file shall be called, skip file not exists + {"mock_200_4", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists + {"mock_200_5", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called +} + +func NewSkipFileTest() *MockHelperImpl { + var ret MockHelperImpl + ret.expected_steps = test_expected_scenario + return &ret +} + +func (h MockHelperImpl) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { + ret := []oidWithBatch{ + {100, 2}, + {200, 3}, + {200, 4}, + {200, 5}, + } + return ret, nil +} + +func (h MockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { + + step := h.getCurStep() + Expect(tableOid).To(Equal(step.check_skip_file_arg_tableoid)) + ret := h.getCurStep().check_skip_file_result + return ret +} + +func (h MockHelperImpl) createPipe(pipe string) error { + return nil +} +func (h *MockHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (*RestoreReader, error) { + return nil, errors.New("getRestoreDataReader Not implemented") +} + +func (h *MockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { + + h.step_no++ + Expect(currentPipe).To(Equal(h.expected_steps[h.step_no].getRestorePipeWriter_arg_expect)) + + if test_expected_scenario[h.step_no].getRestorePipeWriter_result { + var writer bufio.Writer + return &writer, nil, nil + } + return nil, nil, unix.ENXIO +} + var _ = Describe("helper tests", func() { var pluginConfig utils.PluginConfig var isSubset bool @@ -98,4 +173,44 @@ var _ = Describe("helper tests", func() { Expect(isSubset).To(Equal(false)) }) }) + + Describe("doRestoreAgent", func() { + It("Test skip file in doRestoreAgent", func() { + Expect(1).To(Equal(1)) + + save_singleDataFile := *singleDataFile + save_content := *content + save_oidFile := *oidFile + save_isResizeRestore := *isResizeRestore + save_origSize := *origSize + save_destSize := *destSize + save_pipeFile := *pipeFile + save_onErrorContinue := *onErrorContinue + + *singleDataFile = false + *content = 1 + *oidFile = "testoid.dat" + *isResizeRestore = true + *origSize = 1 + *destSize = 1 + *pipeFile = "mock" + *onErrorContinue = true + + helper := NewSkipFileTest() + + err := doRestoreAgent_internal(helper, helper) + + Expect(err).To(BeNil()) + + *singleDataFile = save_singleDataFile + *content = save_content + *oidFile = save_oidFile + *isResizeRestore = save_isResizeRestore + *origSize = save_origSize + *destSize = save_destSize + *pipeFile = save_pipeFile + *onErrorContinue = save_onErrorContinue + + }) + }) }) diff --git a/helper/restore_helper.go b/helper/restore_helper.go index c188b6494..61c85204a 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -123,7 +123,19 @@ type oidWithBatch struct { batch int } -func doRestoreAgent() error { +type RestoreHelper interface { + getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (*RestoreReader, error) + getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) + checkForSkipFile(pipeFile string, tableOid int) bool +} + +type RestoreHelperImpl struct{} + +func (RestoreHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { + return utils.FileExists(fmt.Sprintf("%s_skip_%d", pipeFile, tableOid)) +} + +func doRestoreAgent_internal(h Helper, rh RestoreHelper) error { // We need to track various values separately per content for resize restore var segmentTOC map[int]*toc.SegmentTOC var tocEntries map[int]map[uint]toc.SegmentDataEntry @@ -136,7 +148,7 @@ func doRestoreAgent() error { readers := make(map[int]*RestoreReader) - oidWithBatchList, err := getOidWithBatchListFromFile(*oidFile) + oidWithBatchList, err := h.getOidWithBatchListFromFile(*oidFile) if err != nil { return err } @@ -184,7 +196,7 @@ func doRestoreAgent() error { tocEntries[contentToRestore] = segmentTOC[contentToRestore].DataEntries filename := replaceContentInFilename(*dataFile, contentToRestore) - readers[contentToRestore], err = getRestoreDataReader(filename, segmentTOC[contentToRestore], oidList) + readers[contentToRestore], err = rh.getRestoreDataReader(filename, segmentTOC[contentToRestore], oidList) if readers[contentToRestore] != nil { // NOTE: If we reach here with batches > 1, there will be // *origSize / *destSize (N old segments / N new segments) @@ -229,7 +241,7 @@ func doRestoreAgent() error { nextBatchNum := nextOidWithBatch.batch nextPipeToCreate := fmt.Sprintf("%s_%d_%d", *pipeFile, nextOid, nextBatchNum) logVerbose(fmt.Sprintf("Oid %d, Batch %d: Creating pipe %s\n", nextOid, nextBatchNum, nextPipeToCreate)) - err := createPipe(nextPipeToCreate) + err := h.createPipe(nextPipeToCreate) if err != nil { logError(fmt.Sprintf("Oid %d, Batch %d: Failed to create pipe %s\n", nextOid, nextBatchNum, nextPipeToCreate)) // In the case this error is hit it means we have lost the @@ -256,7 +268,7 @@ func doRestoreAgent() error { // We pre-create readers above for the sake of not re-opening SDF readers. For MDF we can't // re-use them but still having them in a map simplifies overall code flow. We repeatedly assign // to a map entry here intentionally. - readers[contentToRestore], err = getRestoreDataReader(filename, nil, nil) + readers[contentToRestore], err = rh.getRestoreDataReader(filename, nil, nil) if err != nil { logError(fmt.Sprintf("Oid: %d, Batch %d: Error encountered getting restore data reader: %v", tableOid, batchNum, err)) return err @@ -266,7 +278,7 @@ func doRestoreAgent() error { logInfo(fmt.Sprintf("Oid %d, Batch %d: Opening pipe %s", tableOid, batchNum, currentPipe)) for { - writer, writeHandle, err = getRestorePipeWriter(currentPipe) + writer, writeHandle, err = rh.getRestorePipeWriter(currentPipe) if err != nil { if errors.Is(err, unix.ENXIO) { // COPY (the pipe reader) has not tried to access the pipe yet so our restore_helper @@ -277,7 +289,7 @@ func doRestoreAgent() error { // might be good to have a GPDB version check here. However, the restore helper should // not contain a database connection so the version should be passed through the helper // invocation from gprestore (e.g. create a --db-version flag option). - if *onErrorContinue && utils.FileExists(fmt.Sprintf("%s_skip_%d", *pipeFile, tableOid)) { + if *onErrorContinue && rh.checkForSkipFile(*pipeFile, tableOid) { logWarn(fmt.Sprintf("Oid %d, Batch %d: Skip file discovered, skipping this relation.", tableOid, batchNum)) err = nil goto LoopEnd @@ -387,6 +399,12 @@ func doRestoreAgent() error { return lastError } +func doRestoreAgent() error { + helper := new(HelperImpl) + restorer := new(RestoreHelperImpl) + return doRestoreAgent_internal(helper, restorer) +} + func constructSingleTableFilename(name string, contentToRestore int, oid int) string { name = strings.ReplaceAll(name, fmt.Sprintf("gpbackup_%d", *content), fmt.Sprintf("gpbackup_%d", contentToRestore)) nameParts := strings.Split(name, ".") @@ -406,7 +424,7 @@ func replaceContentInFilename(filename string, content int) string { return contentRE.ReplaceAllString(filename, fmt.Sprintf("gpbackup_%d_", content)) } -func getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (*RestoreReader, error) { +func (RestoreHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (*RestoreReader, error) { var readHandle io.Reader var seekHandle io.ReadSeeker var isSubset bool @@ -470,7 +488,7 @@ func getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []i return restoreReader, err } -func getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { +func (RestoreHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { fileHandle, err := os.OpenFile(currentPipe, os.O_WRONLY|unix.O_NONBLOCK, os.ModeNamedPipe) if err != nil { // error logging handled by calling functions From 9106dc8ff06c8677d278bc151ac6f8732829c7cc Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Wed, 14 Aug 2024 19:31:56 +0300 Subject: [PATCH 02/29] more clear test definition, add checks for pipes existance --- helper/helper.go | 5 +- helper/helper_test.go | 104 +++++++++++++++++++++++++-------------- helper/restore_helper.go | 2 +- 3 files changed, 72 insertions(+), 39 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index ea8c1e9bd..021645358 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -215,6 +215,7 @@ func preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCo type Helper interface { getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) createPipe(pipe string) error + flushAndCloseRestoreWriter(pipeName string, oid int) error } type HelperImpl struct{} @@ -256,7 +257,7 @@ func getOidListFromFile(oidFileName string) ([]int, error) { return oidList, nil } -func flushAndCloseRestoreWriter(pipeName string, oid int) error { +func (h *HelperImpl) flushAndCloseRestoreWriter(pipeName string, oid int) error { if writer != nil { writer.Write([]byte{}) // simulate writer connected in case of error err := writer.Flush() @@ -302,7 +303,7 @@ func DoCleanup() { logVerbose("Encountered error closing error file: %v", err) } } - err := flushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) + err := new(HelperImpl).flushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) if err != nil { logVerbose("Encountered error during cleanup: %v", err) } diff --git a/helper/helper_test.go b/helper/helper_test.go index d7f716022..6c7066fd5 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -14,48 +14,39 @@ import ( "github.com/pkg/errors" ) -type test_step struct { +type SkipFile_test_step struct { getRestorePipeWriter_arg_expect string getRestorePipeWriter_result bool check_skip_file_arg_tableoid int check_skip_file_result bool } -type MockHelperImpl struct { - step_no int - expected_steps []test_step +type SkipFileMockHelperImpl struct { + step_no int + expected_oidbatch []oidWithBatch + expected_steps []SkipFile_test_step + + opened_pipes map[string]string // Ginkgo matcher works over map value, will diplicate key here } -func (h MockHelperImpl) getCurStep() test_step { +func (h *SkipFileMockHelperImpl) getCurStep() SkipFile_test_step { return h.expected_steps[h.step_no] } -var test_expected_scenario = []test_step{ - {}, // placeholder as steps start from 1 - {"mock_100_2", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called - {"mock_200_3", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called - {"mock_200_4", false, 200, false}, // Can not open pipe for table 200, check_skip_file shall be called, skip file not exists - {"mock_200_4", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists - {"mock_200_5", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called +func NewSkipFileTest(batches []oidWithBatch, steps []SkipFile_test_step) *SkipFileMockHelperImpl { + var ret = new(SkipFileMockHelperImpl) + ret.expected_oidbatch = batches + ret.expected_steps = steps + ret.opened_pipes = nil + return ret } -func NewSkipFileTest() *MockHelperImpl { - var ret MockHelperImpl - ret.expected_steps = test_expected_scenario - return &ret -} +func (h *SkipFileMockHelperImpl) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { -func (h MockHelperImpl) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { - ret := []oidWithBatch{ - {100, 2}, - {200, 3}, - {200, 4}, - {200, 5}, - } - return ret, nil + return h.expected_oidbatch, nil } -func (h MockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { +func (h *SkipFileMockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { step := h.getCurStep() Expect(tableOid).To(Equal(step.check_skip_file_arg_tableoid)) @@ -63,19 +54,44 @@ func (h MockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { return ret } -func (h MockHelperImpl) createPipe(pipe string) error { +func (h *SkipFileMockHelperImpl) createPipe(pipe string) error { + + // Some pipes are preopened before calling doRestoreAgent. Add them to pipe tracking list + if h.opened_pipes == nil { + h.opened_pipes = map[string]string{} + + for k, _ := range pipesMap { + h.opened_pipes[k] = k + } + } + // Check that pipe was not opened yet + Expect(h.opened_pipes).ShouldNot(ContainElement(pipe)) + + h.opened_pipes[pipe] = pipe return nil } -func (h *MockHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (*RestoreReader, error) { + +func (h *SkipFileMockHelperImpl) flushAndCloseRestoreWriter(pipeName string, oid int) error { + + // Check that we are closing pipe which is opened + Expect(h.opened_pipes).To(ContainElement(pipeName)) + delete(h.opened_pipes, pipeName) + return nil +} + +func (h *SkipFileMockHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (*RestoreReader, error) { return nil, errors.New("getRestoreDataReader Not implemented") } -func (h *MockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { +func (h *SkipFileMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { h.step_no++ - Expect(currentPipe).To(Equal(h.expected_steps[h.step_no].getRestorePipeWriter_arg_expect)) + Expect(currentPipe).To(Equal(h.getCurStep().getRestorePipeWriter_arg_expect)) + + // The pipe should be created before + Expect(h.opened_pipes).Should(ContainElement(currentPipe)) - if test_expected_scenario[h.step_no].getRestorePipeWriter_result { + if h.getCurStep().getRestorePipeWriter_result { var writer bufio.Writer return &writer, nil, nil } @@ -174,7 +190,7 @@ var _ = Describe("helper tests", func() { }) }) - Describe("doRestoreAgent", func() { + Describe("doRestoreAgent Mocked unit tests", func() { It("Test skip file in doRestoreAgent", func() { Expect(1).To(Equal(1)) @@ -191,15 +207,31 @@ var _ = Describe("helper tests", func() { *content = 1 *oidFile = "testoid.dat" *isResizeRestore = true - *origSize = 1 - *destSize = 1 + *origSize = 5 + *destSize = 3 *pipeFile = "mock" *onErrorContinue = true - helper := NewSkipFileTest() + // Test Scenario 1. Simulate 1 pass for the doRestoreAgent() function with the specified oids, bathces and expected calls + oid_batch := []oidWithBatch{ + {100, 2}, + {200, 3}, + {200, 4}, + {200, 5}, + } + + expected_scenario := []SkipFile_test_step{ + {}, // placeholder as steps start from 1 + {"mock_100_2", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called + {"mock_200_3", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called + {"mock_200_4", false, 200, false}, // Can not open pipe for table 200, check_skip_file shall be called, skip file not exists + {"mock_200_4", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists + {"mock_200_5", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called + } + + helper := NewSkipFileTest(oid_batch, expected_scenario) err := doRestoreAgent_internal(helper, helper) - Expect(err).To(BeNil()) *singleDataFile = save_singleDataFile diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 61c85204a..cb1c3bb08 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -360,7 +360,7 @@ func doRestoreAgent_internal(h Helper, rh RestoreHelper) error { LoopEnd: logInfo(fmt.Sprintf("Oid %d, Batch %d: Closing pipe %s", tableOid, batchNum, currentPipe)) - err = flushAndCloseRestoreWriter(currentPipe, tableOid) + err = h.flushAndCloseRestoreWriter(currentPipe, tableOid) if err != nil { logVerbose(fmt.Sprintf("Oid %d, Batch %d: Failed to flush and close pipe: %s", tableOid, batchNum, err)) } From 8d547f7f89149eddffaae4fd259d58152c77e51e Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Wed, 21 Aug 2024 00:59:22 +0500 Subject: [PATCH 03/29] Add two more tests to mocking sample --- helper/helper_test.go | 212 +++++++++++++++++++++++++++++---------- helper/restore_helper.go | 39 +++++-- 2 files changed, 187 insertions(+), 64 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index 6c7066fd5..1c68b02a8 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -2,6 +2,7 @@ package helper import ( "bufio" + "fmt" "os" "github.com/greenplum-db/gpbackup/utils" @@ -14,39 +15,85 @@ import ( "github.com/pkg/errors" ) -type SkipFile_test_step struct { +type RestoreReaderTestImpl struct{} + +func (r *RestoreReaderTestImpl) waitForPlugin() error { + // No plugins yet, no errors to detect + return nil +} + +func (r *RestoreReaderTestImpl) positionReader(pos uint64, oid int) error { + return nil +} + +func (r *RestoreReaderTestImpl) copyData(num int64) (int64, error) { + return 0, errors.New("copyData Not implemented") +} + +func (r *RestoreReaderTestImpl) copyAllData() (int64, error) { + return 0, errors.New("copyAllData Not implemented") +} + +func (r *RestoreReaderTestImpl) getFileHandle() *os.File { + return nil +} + +func (r *RestoreReaderTestImpl) getReaderType() ReaderType { + return "nil" +} + +type SkipFileTestStep struct { getRestorePipeWriter_arg_expect string getRestorePipeWriter_result bool check_skip_file_arg_tableoid int check_skip_file_result bool } -type SkipFileMockHelperImpl struct { +type RestoreMockHelperImpl struct { step_no int expected_oidbatch []oidWithBatch - expected_steps []SkipFile_test_step + expected_steps []SkipFileTestStep - opened_pipes map[string]string // Ginkgo matcher works over map value, will diplicate key here + opened_pipes_map map[string]string // Ginkgo matcher works over map value, will diplicate key here + restoreData *RestoreReaderTestImpl } -func (h *SkipFileMockHelperImpl) getCurStep() SkipFile_test_step { +func (h *RestoreMockHelperImpl) opened_pipes() []string { + + if h.opened_pipes_map == nil { + h.opened_pipes_map = make(map[string]string) + + for k := range pipesMap { + h.opened_pipes_map[k] = k + } + } + ret := make([]string, 0, len(h.opened_pipes_map)) + for k := range h.opened_pipes_map { + ret = append(ret, k) + } + return ret +} + +func (h *RestoreMockHelperImpl) getCurStep() SkipFileTestStep { + Expect(h.step_no).To(BeNumerically("<", len(h.expected_steps))) return h.expected_steps[h.step_no] } -func NewSkipFileTest(batches []oidWithBatch, steps []SkipFile_test_step) *SkipFileMockHelperImpl { - var ret = new(SkipFileMockHelperImpl) +func NewSkipFileTest(batches []oidWithBatch, steps []SkipFileTestStep) *RestoreMockHelperImpl { + var ret = new(RestoreMockHelperImpl) ret.expected_oidbatch = batches ret.expected_steps = steps - ret.opened_pipes = nil + ret.opened_pipes_map = nil + ret.restoreData = nil + return ret } -func (h *SkipFileMockHelperImpl) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { - +func (h *RestoreMockHelperImpl) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { return h.expected_oidbatch, nil } -func (h *SkipFileMockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { +func (h *RestoreMockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { step := h.getCurStep() Expect(tableOid).To(Equal(step.check_skip_file_arg_tableoid)) @@ -54,42 +101,35 @@ func (h *SkipFileMockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) return ret } -func (h *SkipFileMockHelperImpl) createPipe(pipe string) error { - - // Some pipes are preopened before calling doRestoreAgent. Add them to pipe tracking list - if h.opened_pipes == nil { - h.opened_pipes = map[string]string{} - - for k, _ := range pipesMap { - h.opened_pipes[k] = k - } - } +func (h *RestoreMockHelperImpl) createPipe(pipe string) error { // Check that pipe was not opened yet - Expect(h.opened_pipes).ShouldNot(ContainElement(pipe)) + Expect(h.opened_pipes()).ShouldNot(ContainElement(pipe)) - h.opened_pipes[pipe] = pipe + h.opened_pipes_map[pipe] = pipe return nil } -func (h *SkipFileMockHelperImpl) flushAndCloseRestoreWriter(pipeName string, oid int) error { +func (h *RestoreMockHelperImpl) flushAndCloseRestoreWriter(pipeName string, oid int) error { // Check that we are closing pipe which is opened - Expect(h.opened_pipes).To(ContainElement(pipeName)) - delete(h.opened_pipes, pipeName) + Expect(h.opened_pipes()).To(ContainElement(pipeName)) + delete(h.opened_pipes_map, pipeName) return nil } -func (h *SkipFileMockHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (*RestoreReader, error) { +func (h *RestoreMockHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (RestoreReader, error) { + if h.restoreData != nil { + return h.restoreData, nil + } return nil, errors.New("getRestoreDataReader Not implemented") } -func (h *SkipFileMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { - +func (h *RestoreMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { h.step_no++ Expect(currentPipe).To(Equal(h.getCurStep().getRestorePipeWriter_arg_expect)) // The pipe should be created before - Expect(h.opened_pipes).Should(ContainElement(currentPipe)) + Expect(h.opened_pipes()).Should(ContainElement(currentPipe)) if h.getCurStep().getRestorePipeWriter_result { var writer bufio.Writer @@ -191,17 +231,26 @@ var _ = Describe("helper tests", func() { }) Describe("doRestoreAgent Mocked unit tests", func() { - It("Test skip file in doRestoreAgent", func() { - Expect(1).To(Equal(1)) - - save_singleDataFile := *singleDataFile - save_content := *content - save_oidFile := *oidFile - save_isResizeRestore := *isResizeRestore - save_origSize := *origSize - save_destSize := *destSize - save_pipeFile := *pipeFile - save_onErrorContinue := *onErrorContinue + var ( + save_singleDataFile bool + save_content int + save_oidFile string + save_isResizeRestore bool + save_origSize int + save_destSize int + save_pipeFile string + save_onErrorContinue bool + ) + + BeforeEach(func() { + save_singleDataFile = *singleDataFile + save_content = *content + save_oidFile = *oidFile + save_isResizeRestore = *isResizeRestore + save_origSize = *origSize + save_destSize = *destSize + save_pipeFile = *pipeFile + save_onErrorContinue = *onErrorContinue *singleDataFile = false *content = 1 @@ -211,8 +260,75 @@ var _ = Describe("helper tests", func() { *destSize = 3 *pipeFile = "mock" *onErrorContinue = true + }) + + AfterEach(func() { + *singleDataFile = save_singleDataFile + *content = save_content + *oidFile = save_oidFile + *isResizeRestore = save_isResizeRestore + *origSize = save_origSize + *destSize = save_destSize + *pipeFile = save_pipeFile + *onErrorContinue = save_onErrorContinue + }) + It("successfully restores using a single data file when inputs are valid and no errors occur", func() { + *singleDataFile = true + + oid_batch := []oidWithBatch{{oid: 1, batch: 1}} + steps := []SkipFileTestStep{ + {}, + {getRestorePipeWriter_arg_expect: "mock_1_1", getRestorePipeWriter_result: true, check_skip_file_arg_tableoid: 1, check_skip_file_result: false}, + } + + mockHelper := NewSkipFileTest(oid_batch, steps) + mockHelper.restoreData = &RestoreReaderTestImpl{} + + // Prepare the toc file + testDir := "" //"/tmp/helper_test/20180101/20180101010101/" + *tocFile = fmt.Sprintf("%stest_toc.yaml", testDir) + dataLength := 128*1024 + 1 + // Write custom TOC + customTOC := fmt.Sprintf(`dataentries: + 1: + startbyte: 0 + endbyte: 18 + 2: + startbyte: 18 + endbyte: %[1]d + 3: + startbyte: %[1]d + endbyte: %d +`, dataLength+18, dataLength+18+18) + fToc, _ := os.Create(*tocFile) + _, _ = fToc.WriteString(customTOC) + defer func() { + _ = os.Remove(*tocFile) + }() + + *dataFile = "test_data.dat" + // Call the function under test + err := doRestoreAgent_internal(mockHelper, mockHelper) + + Expect(err).ToNot(HaveOccurred()) + }) + It("successfully restores using multiple data files when inputs are valid and no errors occur", func() { + // Call the function under test + oid_batch := []oidWithBatch{{oid: 1, batch: 1}} + steps := []SkipFileTestStep{ + {}, + {getRestorePipeWriter_arg_expect: "mock_1_1", getRestorePipeWriter_result: true, check_skip_file_arg_tableoid: 1, check_skip_file_result: false}, + } + + mockHelper := NewSkipFileTest(oid_batch, steps) + mockHelper.restoreData = &RestoreReaderTestImpl{} + + err := doRestoreAgent_internal(mockHelper, mockHelper) - // Test Scenario 1. Simulate 1 pass for the doRestoreAgent() function with the specified oids, bathces and expected calls + Expect(err).ToNot(HaveOccurred()) + }) + It("skips batches with corresponding skip file in doRestoreAgent", func() { + // Test Scenario 1. Simulate 1 pass for the doRestoreAgent() function with the specified oids, batches and expected calls oid_batch := []oidWithBatch{ {100, 2}, {200, 3}, @@ -220,7 +336,7 @@ var _ = Describe("helper tests", func() { {200, 5}, } - expected_scenario := []SkipFile_test_step{ + expected_scenario := []SkipFileTestStep{ {}, // placeholder as steps start from 1 {"mock_100_2", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called {"mock_200_3", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called @@ -233,16 +349,6 @@ var _ = Describe("helper tests", func() { err := doRestoreAgent_internal(helper, helper) Expect(err).To(BeNil()) - - *singleDataFile = save_singleDataFile - *content = save_content - *oidFile = save_oidFile - *isResizeRestore = save_isResizeRestore - *origSize = save_origSize - *destSize = save_destSize - *pipeFile = save_pipeFile - *onErrorContinue = save_onErrorContinue - }) }) }) diff --git a/helper/restore_helper.go b/helper/restore_helper.go index cb1c3bb08..ea2334651 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -43,7 +43,16 @@ var ( * SUBSET type applies when restoring using plugin(if compatible) from uncompressed data with filters * NONSEEKABLE type applies for every other restore scenario */ -type RestoreReader struct { +type RestoreReader interface { + waitForPlugin() error + positionReader(pos uint64, oid int) error + copyData(num int64) (int64, error) + copyAllData() (int64, error) + getFileHandle() *os.File + getReaderType() ReaderType +} + +type RestoreReaderImpl struct { fileHandle *os.File bufReader *bufio.Reader seekReader io.ReadSeeker @@ -53,7 +62,7 @@ type RestoreReader struct { // Wait for plugin process that should be already finished. This should be // called on every reader that used a plugin as to not leave any zombies behind. -func (r *RestoreReader) waitForPlugin() error { +func (r *RestoreReaderImpl) waitForPlugin() error { var err error if r.pluginCmd != nil && r.pluginCmd.Process != nil { logVerbose(fmt.Sprintf("Waiting for the plugin process (%d)", r.pluginCmd.Process.Pid)) @@ -72,7 +81,7 @@ func (r *RestoreReader) waitForPlugin() error { return err } -func (r *RestoreReader) positionReader(pos uint64, oid int) error { +func (r *RestoreReaderImpl) positionReader(pos uint64, oid int) error { switch r.readerType { case SEEKABLE: seekPosition, err := r.seekReader.Seek(int64(pos), io.SeekCurrent) @@ -94,7 +103,7 @@ func (r *RestoreReader) positionReader(pos uint64, oid int) error { return nil } -func (r *RestoreReader) copyData(num int64) (int64, error) { +func (r *RestoreReaderImpl) copyData(num int64) (int64, error) { var bytesRead int64 var err error switch r.readerType { @@ -106,7 +115,7 @@ func (r *RestoreReader) copyData(num int64) (int64, error) { return bytesRead, err } -func (r *RestoreReader) copyAllData() (int64, error) { +func (r *RestoreReaderImpl) copyAllData() (int64, error) { var bytesRead int64 var err error switch r.readerType { @@ -118,13 +127,21 @@ func (r *RestoreReader) copyAllData() (int64, error) { return bytesRead, err } +func (r *RestoreReaderImpl) getReaderType() ReaderType { + return r.readerType +} + +func (r *RestoreReaderImpl) getFileHandle() *os.File { + return r.fileHandle +} + type oidWithBatch struct { oid int batch int } type RestoreHelper interface { - getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (*RestoreReader, error) + getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (RestoreReader, error) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) checkForSkipFile(pipeFile string, tableOid int) bool } @@ -146,7 +163,7 @@ func doRestoreAgent_internal(h Helper, rh RestoreHelper) error { var bytesRead int64 var lastError error - readers := make(map[int]*RestoreReader) + readers := make(map[int]RestoreReader) oidWithBatchList, err := h.getOidWithBatchListFromFile(*oidFile) if err != nil { @@ -215,7 +232,7 @@ func doRestoreAgent_internal(h Helper, rh RestoreHelper) error { logError(fmt.Sprintf("Error encountered getting restore data reader for single data file: %v", err)) return err } - logVerbose(fmt.Sprintf("Using reader type: %s", readers[contentToRestore].readerType)) + logVerbose(fmt.Sprintf("Using reader type: %s", readers[contentToRestore].getReaderType())) contentToRestore += *destSize } @@ -263,7 +280,7 @@ func doRestoreAgent_internal(h Helper, rh RestoreHelper) error { // Close file before it gets overwritten. Free up these // resources when the reader is not needed anymore. if reader, ok := readers[contentToRestore]; ok { - reader.fileHandle.Close() + reader.getFileHandle().Close() } // We pre-create readers above for the sake of not re-opening SDF readers. For MDF we can't // re-use them but still having them in a map simplifies overall code flow. We repeatedly assign @@ -424,13 +441,13 @@ func replaceContentInFilename(filename string, content int) string { return contentRE.ReplaceAllString(filename, fmt.Sprintf("gpbackup_%d_", content)) } -func (RestoreHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (*RestoreReader, error) { +func (RestoreHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (RestoreReader, error) { var readHandle io.Reader var seekHandle io.ReadSeeker var isSubset bool var err error = nil var pluginCmd *PluginCmd = nil - restoreReader := new(RestoreReader) + restoreReader := new(RestoreReaderImpl) if *pluginConfigFile != "" { pluginCmd, readHandle, isSubset, err = startRestorePluginCommand(fileToRead, objToc, oidList) From 26810133377f9fcb3bf7ff69a2642c1fc1b109e2 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Wed, 21 Aug 2024 18:01:20 +0500 Subject: [PATCH 04/29] Fix after merge --- helper/restore_helper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 09a407f8a..1444e9cef 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -377,7 +377,7 @@ func doRestoreAgent_internal(h Helper, rh RestoreHelper) error { LoopEnd: logInfo(fmt.Sprintf("Oid %d, Batch %d: Closing pipe %s", tableOid, batchNum, currentPipe)) - errPipe = h.flushAndCloseRestoreWriter(currentPipe, tableOid) + errPipe := h.flushAndCloseRestoreWriter(currentPipe, tableOid) if errPipe != nil { logVerbose(fmt.Sprintf("Oid %d, Batch %d: Failed to flush and close pipe: %s", tableOid, batchNum, errPipe)) } From 68b4fcd940ec878302f0cf395dd40f54a56aaf72 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Thu, 22 Aug 2024 09:24:28 +0500 Subject: [PATCH 05/29] implement mocked methods --- helper/helper_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index 1c68b02a8..aff4911cd 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -27,11 +27,11 @@ func (r *RestoreReaderTestImpl) positionReader(pos uint64, oid int) error { } func (r *RestoreReaderTestImpl) copyData(num int64) (int64, error) { - return 0, errors.New("copyData Not implemented") + return 1, nil } func (r *RestoreReaderTestImpl) copyAllData() (int64, error) { - return 0, errors.New("copyAllData Not implemented") + return 1, nil } func (r *RestoreReaderTestImpl) getFileHandle() *os.File { From 189a078811539c92512389debbae4af1bd9778ae Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Mon, 26 Aug 2024 09:34:45 +0500 Subject: [PATCH 06/29] Change variable case. --- helper/helper_test.go | 123 +++++++++++++++++++++--------------------- 1 file changed, 60 insertions(+), 63 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index aff4911cd..f3efc14ee 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -43,61 +43,59 @@ func (r *RestoreReaderTestImpl) getReaderType() ReaderType { } type SkipFileTestStep struct { - getRestorePipeWriter_arg_expect string - getRestorePipeWriter_result bool - check_skip_file_arg_tableoid int - check_skip_file_result bool + getRestorePipeWriterArgExpect string + getRestorePipeWriterResult bool + checkSkipFileArgTableOid int + checkSkipFileResult bool } type RestoreMockHelperImpl struct { - step_no int - expected_oidbatch []oidWithBatch - expected_steps []SkipFileTestStep + stepNo int + expectedOidBatch []oidWithBatch + expectedSteps []SkipFileTestStep - opened_pipes_map map[string]string // Ginkgo matcher works over map value, will diplicate key here - restoreData *RestoreReaderTestImpl + openedPipesMap map[string]string // Ginkgo matcher works over map value, will diplicate key here + restoreData *RestoreReaderTestImpl } func (h *RestoreMockHelperImpl) opened_pipes() []string { - - if h.opened_pipes_map == nil { - h.opened_pipes_map = make(map[string]string) + if h.openedPipesMap == nil { + h.openedPipesMap = make(map[string]string) for k := range pipesMap { - h.opened_pipes_map[k] = k + h.openedPipesMap[k] = k } } - ret := make([]string, 0, len(h.opened_pipes_map)) - for k := range h.opened_pipes_map { + ret := make([]string, 0, len(h.openedPipesMap)) + for k := range h.openedPipesMap { ret = append(ret, k) } return ret } func (h *RestoreMockHelperImpl) getCurStep() SkipFileTestStep { - Expect(h.step_no).To(BeNumerically("<", len(h.expected_steps))) - return h.expected_steps[h.step_no] + Expect(h.stepNo).To(BeNumerically("<", len(h.expectedSteps))) + return h.expectedSteps[h.stepNo] } func NewSkipFileTest(batches []oidWithBatch, steps []SkipFileTestStep) *RestoreMockHelperImpl { var ret = new(RestoreMockHelperImpl) - ret.expected_oidbatch = batches - ret.expected_steps = steps - ret.opened_pipes_map = nil + ret.expectedOidBatch = batches + ret.expectedSteps = steps + ret.openedPipesMap = nil ret.restoreData = nil return ret } func (h *RestoreMockHelperImpl) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { - return h.expected_oidbatch, nil + return h.expectedOidBatch, nil } func (h *RestoreMockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { - step := h.getCurStep() - Expect(tableOid).To(Equal(step.check_skip_file_arg_tableoid)) - ret := h.getCurStep().check_skip_file_result + Expect(tableOid).To(Equal(step.checkSkipFileArgTableOid)) + ret := h.getCurStep().checkSkipFileResult return ret } @@ -105,15 +103,14 @@ func (h *RestoreMockHelperImpl) createPipe(pipe string) error { // Check that pipe was not opened yet Expect(h.opened_pipes()).ShouldNot(ContainElement(pipe)) - h.opened_pipes_map[pipe] = pipe + h.openedPipesMap[pipe] = pipe return nil } func (h *RestoreMockHelperImpl) flushAndCloseRestoreWriter(pipeName string, oid int) error { - // Check that we are closing pipe which is opened Expect(h.opened_pipes()).To(ContainElement(pipeName)) - delete(h.opened_pipes_map, pipeName) + delete(h.openedPipesMap, pipeName) return nil } @@ -125,13 +122,13 @@ func (h *RestoreMockHelperImpl) getRestoreDataReader(fileToRead string, objToc * } func (h *RestoreMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { - h.step_no++ - Expect(currentPipe).To(Equal(h.getCurStep().getRestorePipeWriter_arg_expect)) + h.stepNo++ + Expect(currentPipe).To(Equal(h.getCurStep().getRestorePipeWriterArgExpect)) // The pipe should be created before Expect(h.opened_pipes()).Should(ContainElement(currentPipe)) - if h.getCurStep().getRestorePipeWriter_result { + if h.getCurStep().getRestorePipeWriterResult { var writer bufio.Writer return &writer, nil, nil } @@ -232,25 +229,25 @@ var _ = Describe("helper tests", func() { Describe("doRestoreAgent Mocked unit tests", func() { var ( - save_singleDataFile bool - save_content int - save_oidFile string - save_isResizeRestore bool - save_origSize int - save_destSize int - save_pipeFile string - save_onErrorContinue bool + saveSingleDataFile bool + saveContent int + saveOidFile string + saveIsResizeRestore bool + saveOrigSize int + saveDestSize int + savePipeFile string + saveOnErrorContinue bool ) BeforeEach(func() { - save_singleDataFile = *singleDataFile - save_content = *content - save_oidFile = *oidFile - save_isResizeRestore = *isResizeRestore - save_origSize = *origSize - save_destSize = *destSize - save_pipeFile = *pipeFile - save_onErrorContinue = *onErrorContinue + saveSingleDataFile = *singleDataFile + saveContent = *content + saveOidFile = *oidFile + saveIsResizeRestore = *isResizeRestore + saveOrigSize = *origSize + saveDestSize = *destSize + savePipeFile = *pipeFile + saveOnErrorContinue = *onErrorContinue *singleDataFile = false *content = 1 @@ -263,25 +260,25 @@ var _ = Describe("helper tests", func() { }) AfterEach(func() { - *singleDataFile = save_singleDataFile - *content = save_content - *oidFile = save_oidFile - *isResizeRestore = save_isResizeRestore - *origSize = save_origSize - *destSize = save_destSize - *pipeFile = save_pipeFile - *onErrorContinue = save_onErrorContinue + *singleDataFile = saveSingleDataFile + *content = saveContent + *oidFile = saveOidFile + *isResizeRestore = saveIsResizeRestore + *origSize = saveOrigSize + *destSize = saveDestSize + *pipeFile = savePipeFile + *onErrorContinue = saveOnErrorContinue }) It("successfully restores using a single data file when inputs are valid and no errors occur", func() { *singleDataFile = true - oid_batch := []oidWithBatch{{oid: 1, batch: 1}} + oidBatch := []oidWithBatch{{oid: 1, batch: 1}} steps := []SkipFileTestStep{ {}, - {getRestorePipeWriter_arg_expect: "mock_1_1", getRestorePipeWriter_result: true, check_skip_file_arg_tableoid: 1, check_skip_file_result: false}, + {getRestorePipeWriterArgExpect: "mock_1_1", getRestorePipeWriterResult: true, checkSkipFileArgTableOid: 1, checkSkipFileResult: false}, } - mockHelper := NewSkipFileTest(oid_batch, steps) + mockHelper := NewSkipFileTest(oidBatch, steps) mockHelper.restoreData = &RestoreReaderTestImpl{} // Prepare the toc file @@ -314,13 +311,13 @@ var _ = Describe("helper tests", func() { }) It("successfully restores using multiple data files when inputs are valid and no errors occur", func() { // Call the function under test - oid_batch := []oidWithBatch{{oid: 1, batch: 1}} + oidBatch := []oidWithBatch{{oid: 1, batch: 1}} steps := []SkipFileTestStep{ {}, - {getRestorePipeWriter_arg_expect: "mock_1_1", getRestorePipeWriter_result: true, check_skip_file_arg_tableoid: 1, check_skip_file_result: false}, + {getRestorePipeWriterArgExpect: "mock_1_1", getRestorePipeWriterResult: true, checkSkipFileArgTableOid: 1, checkSkipFileResult: false}, } - mockHelper := NewSkipFileTest(oid_batch, steps) + mockHelper := NewSkipFileTest(oidBatch, steps) mockHelper.restoreData = &RestoreReaderTestImpl{} err := doRestoreAgent_internal(mockHelper, mockHelper) @@ -329,14 +326,14 @@ var _ = Describe("helper tests", func() { }) It("skips batches with corresponding skip file in doRestoreAgent", func() { // Test Scenario 1. Simulate 1 pass for the doRestoreAgent() function with the specified oids, batches and expected calls - oid_batch := []oidWithBatch{ + oidBatch := []oidWithBatch{ {100, 2}, {200, 3}, {200, 4}, {200, 5}, } - expected_scenario := []SkipFileTestStep{ + expectedScenario := []SkipFileTestStep{ {}, // placeholder as steps start from 1 {"mock_100_2", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called {"mock_200_3", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called @@ -345,7 +342,7 @@ var _ = Describe("helper tests", func() { {"mock_200_5", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called } - helper := NewSkipFileTest(oid_batch, expected_scenario) + helper := NewSkipFileTest(oidBatch, expectedScenario) err := doRestoreAgent_internal(helper, helper) Expect(err).To(BeNil()) From 35e7741e66afa77e29b95dc9a093a4bec9eae2a1 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Tue, 27 Aug 2024 14:14:10 +0500 Subject: [PATCH 07/29] Rename symbols to follow code style --- helper/helper_test.go | 18 +++++++++--------- helper/restore_helper.go | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index f3efc14ee..43e46313b 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -58,7 +58,7 @@ type RestoreMockHelperImpl struct { restoreData *RestoreReaderTestImpl } -func (h *RestoreMockHelperImpl) opened_pipes() []string { +func (h *RestoreMockHelperImpl) openedPipes() []string { if h.openedPipesMap == nil { h.openedPipesMap = make(map[string]string) @@ -101,7 +101,7 @@ func (h *RestoreMockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) func (h *RestoreMockHelperImpl) createPipe(pipe string) error { // Check that pipe was not opened yet - Expect(h.opened_pipes()).ShouldNot(ContainElement(pipe)) + Expect(h.openedPipes()).ShouldNot(ContainElement(pipe)) h.openedPipesMap[pipe] = pipe return nil @@ -109,7 +109,7 @@ func (h *RestoreMockHelperImpl) createPipe(pipe string) error { func (h *RestoreMockHelperImpl) flushAndCloseRestoreWriter(pipeName string, oid int) error { // Check that we are closing pipe which is opened - Expect(h.opened_pipes()).To(ContainElement(pipeName)) + Expect(h.openedPipes()).To(ContainElement(pipeName)) delete(h.openedPipesMap, pipeName) return nil } @@ -126,7 +126,7 @@ func (h *RestoreMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio Expect(currentPipe).To(Equal(h.getCurStep().getRestorePipeWriterArgExpect)) // The pipe should be created before - Expect(h.opened_pipes()).Should(ContainElement(currentPipe)) + Expect(h.openedPipes()).Should(ContainElement(currentPipe)) if h.getCurStep().getRestorePipeWriterResult { var writer bufio.Writer @@ -298,14 +298,14 @@ var _ = Describe("helper tests", func() { endbyte: %d `, dataLength+18, dataLength+18+18) fToc, _ := os.Create(*tocFile) - _, _ = fToc.WriteString(customTOC) + fToc.WriteString(customTOC) defer func() { - _ = os.Remove(*tocFile) + os.Remove(*tocFile) }() *dataFile = "test_data.dat" // Call the function under test - err := doRestoreAgent_internal(mockHelper, mockHelper) + err := doRestoreAgentInternal(mockHelper, mockHelper) Expect(err).ToNot(HaveOccurred()) }) @@ -320,7 +320,7 @@ var _ = Describe("helper tests", func() { mockHelper := NewSkipFileTest(oidBatch, steps) mockHelper.restoreData = &RestoreReaderTestImpl{} - err := doRestoreAgent_internal(mockHelper, mockHelper) + err := doRestoreAgentInternal(mockHelper, mockHelper) Expect(err).ToNot(HaveOccurred()) }) @@ -344,7 +344,7 @@ var _ = Describe("helper tests", func() { helper := NewSkipFileTest(oidBatch, expectedScenario) - err := doRestoreAgent_internal(helper, helper) + err := doRestoreAgentInternal(helper, helper) Expect(err).To(BeNil()) }) }) diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 1444e9cef..9447b90ac 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -152,7 +152,7 @@ func (RestoreHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { return utils.FileExists(fmt.Sprintf("%s_skip_%d", pipeFile, tableOid)) } -func doRestoreAgent_internal(h Helper, rh RestoreHelper) error { +func doRestoreAgentInternal(h Helper, rh RestoreHelper) error { // We need to track various values separately per content for resize restore var segmentTOC map[int]*toc.SegmentTOC var tocEntries map[int]map[uint]toc.SegmentDataEntry @@ -419,7 +419,7 @@ func doRestoreAgent_internal(h Helper, rh RestoreHelper) error { func doRestoreAgent() error { helper := new(HelperImpl) restorer := new(RestoreHelperImpl) - return doRestoreAgent_internal(helper, restorer) + return doRestoreAgentInternal(helper, restorer) } func constructSingleTableFilename(name string, contentToRestore int, oid int) string { From 381cf2b5cf65b7c402850512b9bd2e023b4baaf5 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Wed, 28 Aug 2024 17:10:49 +0500 Subject: [PATCH 08/29] Save all global variables on upper level for tests --- helper/helper_test.go | 98 +++++++++++++++++++++++++++++-------------- 1 file changed, 67 insertions(+), 31 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index 43e46313b..4bd9a47ed 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -135,6 +135,28 @@ func (h *RestoreMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio return nil, nil, unix.ENXIO } +var ( + saveBackupAgent bool + saveCompressionLevel int + saveCompressionType string + saveContent int + saveDataFile string + saveOidFile string + saveOnErrorContinue bool + savePipeFile string + savePluginConfigFile string + savePrintVersion bool + saveRestoreAgent bool + saveTocFile string + saveIsFiltered bool + saveCopyQueue int + saveSingleDataFile bool + saveIsResizeRestore bool + saveOrigSize int + saveDestSize int + saveVerbosity int +) + var _ = Describe("helper tests", func() { var pluginConfig utils.PluginConfig var isSubset bool @@ -152,6 +174,50 @@ var _ = Describe("helper tests", func() { Options: make(map[string]string), } + BeforeEach(func() { + saveBackupAgent = *backupAgent + saveCompressionLevel = *compressionLevel + saveCompressionType = *compressionType + saveContent = *content + saveDataFile = *dataFile + saveOidFile = *oidFile + saveOnErrorContinue = *onErrorContinue + savePipeFile = *pipeFile + savePluginConfigFile = *pluginConfigFile + savePrintVersion = *printVersion + saveRestoreAgent = *restoreAgent + saveTocFile = *tocFile + saveIsFiltered = *isFiltered + saveCopyQueue = *copyQueue + saveSingleDataFile = *singleDataFile + saveIsResizeRestore = *isResizeRestore + saveOrigSize = *origSize + saveDestSize = *destSize + saveVerbosity = *verbosity + }) + + AfterEach(func() { + *backupAgent = saveBackupAgent + *compressionLevel = saveCompressionLevel + *compressionType = saveCompressionType + *content = saveContent + *dataFile = saveDataFile + *oidFile = saveOidFile + *onErrorContinue = saveOnErrorContinue + *pipeFile = savePipeFile + *pluginConfigFile = savePluginConfigFile + *printVersion = savePrintVersion + *restoreAgent = saveRestoreAgent + *tocFile = saveTocFile + *isFiltered = saveIsFiltered + *copyQueue = saveCopyQueue + *singleDataFile = saveSingleDataFile + *isResizeRestore = saveIsResizeRestore + *origSize = saveOrigSize + *destSize = saveDestSize + *verbosity = saveVerbosity + }) + Describe("Check subset flag", func() { It("when restore_subset is off, --on-error-continue is false, compression is not used", func() { pluginConfig.Options["restore_subset"] = "off" @@ -228,27 +294,8 @@ var _ = Describe("helper tests", func() { }) Describe("doRestoreAgent Mocked unit tests", func() { - var ( - saveSingleDataFile bool - saveContent int - saveOidFile string - saveIsResizeRestore bool - saveOrigSize int - saveDestSize int - savePipeFile string - saveOnErrorContinue bool - ) - BeforeEach(func() { - saveSingleDataFile = *singleDataFile - saveContent = *content - saveOidFile = *oidFile - saveIsResizeRestore = *isResizeRestore - saveOrigSize = *origSize - saveDestSize = *destSize - savePipeFile = *pipeFile - saveOnErrorContinue = *onErrorContinue - + // Setup mocked tests environment *singleDataFile = false *content = 1 *oidFile = "testoid.dat" @@ -258,17 +305,6 @@ var _ = Describe("helper tests", func() { *pipeFile = "mock" *onErrorContinue = true }) - - AfterEach(func() { - *singleDataFile = saveSingleDataFile - *content = saveContent - *oidFile = saveOidFile - *isResizeRestore = saveIsResizeRestore - *origSize = saveOrigSize - *destSize = saveDestSize - *pipeFile = savePipeFile - *onErrorContinue = saveOnErrorContinue - }) It("successfully restores using a single data file when inputs are valid and no errors occur", func() { *singleDataFile = true From a2ad8245cfdb38f1d5e8f3ae7e0b2869aeee9b6a Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Wed, 4 Sep 2024 10:39:33 +0500 Subject: [PATCH 09/29] Add recent changes from ADBDEV-6012 and a few more tests --- helper/helper_test.go | 108 ++++++++++++++++++++++++++++++++++----- helper/restore_helper.go | 68 ++++++++++++++++-------- 2 files changed, 142 insertions(+), 34 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index 4bd9a47ed..c05bc5f7d 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -15,6 +15,12 @@ import ( "github.com/pkg/errors" ) +var ( + testDir = "/tmp/helper_test/20180101/20180101010101" + testTocFile = fmt.Sprintf("%s/test_toc.yaml", testDir) + examplePluginTestConfig = "/tmp/test_example_plugin_config.yaml" +) + type RestoreReaderTestImpl struct{} func (r *RestoreReaderTestImpl) waitForPlugin() error { @@ -83,7 +89,7 @@ func NewSkipFileTest(batches []oidWithBatch, steps []SkipFileTestStep) *RestoreM ret.expectedOidBatch = batches ret.expectedSteps = steps ret.openedPipesMap = nil - ret.restoreData = nil + ret.restoreData = &RestoreReaderTestImpl{} return ret } @@ -194,6 +200,9 @@ var _ = Describe("helper tests", func() { saveOrigSize = *origSize saveDestSize = *destSize saveVerbosity = *verbosity + + err := os.MkdirAll(testDir, 0777) + Expect(err).ShouldNot(HaveOccurred()) }) AfterEach(func() { @@ -315,7 +324,6 @@ var _ = Describe("helper tests", func() { } mockHelper := NewSkipFileTest(oidBatch, steps) - mockHelper.restoreData = &RestoreReaderTestImpl{} // Prepare the toc file testDir := "" //"/tmp/helper_test/20180101/20180101010101/" @@ -354,7 +362,6 @@ var _ = Describe("helper tests", func() { } mockHelper := NewSkipFileTest(oidBatch, steps) - mockHelper.restoreData = &RestoreReaderTestImpl{} err := doRestoreAgentInternal(mockHelper, mockHelper) @@ -363,19 +370,18 @@ var _ = Describe("helper tests", func() { It("skips batches with corresponding skip file in doRestoreAgent", func() { // Test Scenario 1. Simulate 1 pass for the doRestoreAgent() function with the specified oids, batches and expected calls oidBatch := []oidWithBatch{ - {100, 2}, - {200, 3}, - {200, 4}, - {200, 5}, + {100, 0}, + {200, 0}, + {200, 1}, + {200, 2}, } expectedScenario := []SkipFileTestStep{ - {}, // placeholder as steps start from 1 - {"mock_100_2", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called - {"mock_200_3", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called - {"mock_200_4", false, 200, false}, // Can not open pipe for table 200, check_skip_file shall be called, skip file not exists - {"mock_200_4", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists - {"mock_200_5", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called + {}, // placeholder as steps start from 1 + {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called + {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called + {"mock_200_1", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists + {"mock_200_2", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called } helper := NewSkipFileTest(oidBatch, expectedScenario) @@ -383,5 +389,81 @@ var _ = Describe("helper tests", func() { err := doRestoreAgentInternal(helper, helper) Expect(err).To(BeNil()) }) + It("skips batches if skip file is discovered with resize restore", func() { + *isResizeRestore = true + *origSize = 3 + *destSize = 5 + + oidBatch := []oidWithBatch{ + {100, 0}, + {200, 0}, + {200, 1}, + {200, 2}, + } + + expectedScenario := []SkipFileTestStep{ + {}, // placeholder as steps start from 1 + {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called + {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called + {"mock_200_1", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists + {"mock_200_2", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called + } + + helper := NewSkipFileTest(oidBatch, expectedScenario) + err := doRestoreAgentInternal(helper, helper) + Expect(err).To(BeNil()) + + }) + It("skips batches if skip file is discovered with single datafile", func() { + *singleDataFile = true + *isResizeRestore = false + *tocFile = testTocFile + + // Although pure concept would be to mock TOC file as well, to keep things simpler + // let's use real TOC file here + writeTestTOC(testTocFile) + defer func() { + _ = os.Remove(*tocFile) + }() + + oidBatch := []oidWithBatch{ + {100, 0}, + {200, 0}, + {200, 1}, + {200, 2}, + } + + expectedScenario := []SkipFileTestStep{ + {}, // placeholder as steps start from 1 + {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called + {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called + {"mock_200_1", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists + {"mock_200_2", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called + } + + helper := NewSkipFileTest(oidBatch, expectedScenario) + err := doRestoreAgentInternal(helper, helper) + Expect(err).To(BeNil()) + }) }) }) + +func writeTestTOC(tocFile string) { + // Write test TOC. We are not going to read data using it, so dataLength is a random number + dataLength := 100 + customTOC := fmt.Sprintf(`dataentries: +1: + startbyte: 0 + endbyte: 18 +2: + startbyte: 18 + endbyte: %[1]d +3: + startbyte: %[1]d + endbyte: %d +`, dataLength+18, dataLength+18+18) + fToc, err := os.Create(tocFile) + Expect(err).ShouldNot(HaveOccurred()) + fToc.WriteString(customTOC) + fToc.Close() +} diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 9447b90ac..bd7d664f6 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -135,6 +135,21 @@ func (r *RestoreReaderImpl) getFileHandle() *os.File { return r.fileHandle } +func closeAndDeletePipe(h Helper, tableOid int, batchNum int) { + pipe := fmt.Sprintf("%s_%d_%d", *pipeFile, tableOid, batchNum) + logInfo(fmt.Sprintf("Oid %d, Batch %d: Closing pipe %s", tableOid, batchNum, pipe)) + errPipe := h.flushAndCloseRestoreWriter(pipe, tableOid) + if errPipe != nil { + logVerbose(fmt.Sprintf("Oid %d, Batch %d: Failed to flush and close pipe: %s", tableOid, batchNum, errPipe)) + } + + logVerbose(fmt.Sprintf("Oid %d, Batch %d: Attempt to delete pipe %s", tableOid, batchNum, pipe)) + errPipe = deletePipe(pipe) + if errPipe != nil { + logError("Oid %d, Batch %d: Failed to remove pipe %s: %v", tableOid, batchNum, pipe, errPipe) + } +} + type oidWithBatch struct { oid int batch int @@ -241,6 +256,10 @@ func doRestoreAgentInternal(h Helper, rh RestoreHelper) error { preloadCreatedPipesForRestore(oidWithBatchList, *copyQueue) var currentPipe string + + // If skip file is detected for the particular tableOid, will not process batches related to this oid + skipOid := -1 + for i, oidWithBatch := range oidWithBatchList { tableOid := oidWithBatch.oid batchNum := oidWithBatch.batch @@ -255,19 +274,27 @@ func doRestoreAgentInternal(h Helper, rh RestoreHelper) error { if i < len(oidWithBatchList)-*copyQueue { nextOidWithBatch := oidWithBatchList[i+*copyQueue] nextOid := nextOidWithBatch.oid - nextBatchNum := nextOidWithBatch.batch - nextPipeToCreate := fmt.Sprintf("%s_%d_%d", *pipeFile, nextOid, nextBatchNum) - logVerbose(fmt.Sprintf("Oid %d, Batch %d: Creating pipe %s\n", nextOid, nextBatchNum, nextPipeToCreate)) - err := h.createPipe(nextPipeToCreate) - if err != nil { - logError(fmt.Sprintf("Oid %d, Batch %d: Failed to create pipe %s\n", nextOid, nextBatchNum, nextPipeToCreate)) - // In the case this error is hit it means we have lost the - // ability to create pipes normally, so hard quit even if - // --on-error-continue is given - return err + + if nextOid != skipOid { + nextBatchNum := nextOidWithBatch.batch + nextPipeToCreate := fmt.Sprintf("%s_%d_%d", *pipeFile, nextOid, nextBatchNum) + logVerbose(fmt.Sprintf("Oid %d, Batch %d: Creating pipe %s\n", nextOid, nextBatchNum, nextPipeToCreate)) + err := h.createPipe(nextPipeToCreate) + if err != nil { + logError(fmt.Sprintf("Oid %d, Batch %d: Failed to create pipe %s\n", nextOid, nextBatchNum, nextPipeToCreate)) + // In the case this error is hit it means we have lost the + // ability to create pipes normally, so hard quit even if + // --on-error-continue is given + return err + } } } + if tableOid == skipOid { + logVerbose(fmt.Sprintf("Oid %d, Batch %d: skip due to skip file\n", tableOid, batchNum)) + goto LoopEnd + } + if *singleDataFile { start[contentToRestore] = tocEntries[contentToRestore][uint(tableOid)].StartByte end[contentToRestore] = tocEntries[contentToRestore][uint(tableOid)].EndByte @@ -309,6 +336,14 @@ func doRestoreAgentInternal(h Helper, rh RestoreHelper) error { if *onErrorContinue && rh.checkForSkipFile(*pipeFile, tableOid) { logWarn(fmt.Sprintf("Oid %d, Batch %d: Skip file discovered, skipping this relation.", tableOid, batchNum)) err = nil + skipOid = tableOid + /* Close up to *copyQueue files with this tableOid */ + for idx := 0; idx < *copyQueue; idx++ { + batchToDelete := batchNum + idx + if batchToDelete < batches { + closeAndDeletePipe(h, tableOid, batchToDelete) + } + } goto LoopEnd } else { // keep trying to open the pipe @@ -374,12 +409,9 @@ func doRestoreAgentInternal(h Helper, rh RestoreHelper) error { lastByte[contentToRestore] = end[contentToRestore] } logInfo(fmt.Sprintf("Oid %d, Batch %d: Copied %d bytes into the pipe", tableOid, batchNum, bytesRead)) - LoopEnd: - logInfo(fmt.Sprintf("Oid %d, Batch %d: Closing pipe %s", tableOid, batchNum, currentPipe)) - errPipe := h.flushAndCloseRestoreWriter(currentPipe, tableOid) - if errPipe != nil { - logVerbose(fmt.Sprintf("Oid %d, Batch %d: Failed to flush and close pipe: %s", tableOid, batchNum, errPipe)) + if tableOid != skipOid { + closeAndDeletePipe(h, tableOid, batchNum) } logVerbose(fmt.Sprintf("Oid %d, Batch %d: End batch restore", tableOid, batchNum)) @@ -395,12 +427,6 @@ func doRestoreAgentInternal(h Helper, rh RestoreHelper) error { } } - logVerbose(fmt.Sprintf("Oid %d, Batch %d: Attempt to delete pipe %s", tableOid, batchNum, currentPipe)) - errPipe = deletePipe(currentPipe) - if errPipe != nil { - logError("Oid %d, Batch %d: Failed to remove pipe %s: %v", tableOid, batchNum, currentPipe, errPipe) - } - if err != nil { logError(fmt.Sprintf("Oid %d, Batch %d: Error encountered: %v", tableOid, batchNum, err)) if *onErrorContinue { From aa5d445ff3e2560fa9db9a6be980874c8be3da16 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Wed, 4 Sep 2024 15:29:21 +0500 Subject: [PATCH 10/29] Rename interfaces and structs --- helper/helper.go | 12 +++++------ helper/helper_test.go | 2 +- helper/restore_helper.go | 44 ++++++++++++++++++++-------------------- 3 files changed, 29 insertions(+), 29 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index 021645358..468fe2efa 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -212,19 +212,19 @@ func preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCo } } -type Helper interface { +type IHelper interface { getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) createPipe(pipe string) error flushAndCloseRestoreWriter(pipeName string, oid int) error } -type HelperImpl struct{} +type Helper struct{} -func (h HelperImpl) createPipe(pipe string) error { +func (h Helper) createPipe(pipe string) error { return createPipe(pipe) } -func (h HelperImpl) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { +func (h Helper) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { oidStr, err := operating.System.ReadFile(oidFileName) if err != nil { logError(fmt.Sprintf("Error encountered reading oid batch list from file: %v", err)) @@ -257,7 +257,7 @@ func getOidListFromFile(oidFileName string) ([]int, error) { return oidList, nil } -func (h *HelperImpl) flushAndCloseRestoreWriter(pipeName string, oid int) error { +func (h *Helper) flushAndCloseRestoreWriter(pipeName string, oid int) error { if writer != nil { writer.Write([]byte{}) // simulate writer connected in case of error err := writer.Flush() @@ -303,7 +303,7 @@ func DoCleanup() { logVerbose("Encountered error closing error file: %v", err) } } - err := new(HelperImpl).flushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) + err := new(Helper).flushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) if err != nil { logVerbose("Encountered error during cleanup: %v", err) } diff --git a/helper/helper_test.go b/helper/helper_test.go index c05bc5f7d..2bbc4964a 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -120,7 +120,7 @@ func (h *RestoreMockHelperImpl) flushAndCloseRestoreWriter(pipeName string, oid return nil } -func (h *RestoreMockHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (RestoreReader, error) { +func (h *RestoreMockHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) { if h.restoreData != nil { return h.restoreData, nil } diff --git a/helper/restore_helper.go b/helper/restore_helper.go index bd7d664f6..61c1d0451 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -36,14 +36,14 @@ var ( contentRE *regexp.Regexp ) -/* RestoreReader structure to wrap the underlying reader. +/* IRestoreReader interface to wrap the underlying reader. * readerType identifies how the reader can be used * SEEKABLE uses seekReader. Used when restoring from uncompressed data with filters from local filesystem * NONSEEKABLE and SUBSET types uses bufReader. * SUBSET type applies when restoring using plugin(if compatible) from uncompressed data with filters * NONSEEKABLE type applies for every other restore scenario */ -type RestoreReader interface { +type IRestoreReader interface { waitForPlugin() error positionReader(pos uint64, oid int) error copyData(num int64) (int64, error) @@ -52,7 +52,7 @@ type RestoreReader interface { getReaderType() ReaderType } -type RestoreReaderImpl struct { +type RestoreReader struct { fileHandle *os.File bufReader *bufio.Reader seekReader io.ReadSeeker @@ -62,7 +62,7 @@ type RestoreReaderImpl struct { // Wait for plugin process that should be already finished. This should be // called on every reader that used a plugin as to not leave any zombies behind. -func (r *RestoreReaderImpl) waitForPlugin() error { +func (r *RestoreReader) waitForPlugin() error { var err error if r.pluginCmd != nil && r.pluginCmd.Process != nil { logVerbose(fmt.Sprintf("Waiting for the plugin process (%d)", r.pluginCmd.Process.Pid)) @@ -81,7 +81,7 @@ func (r *RestoreReaderImpl) waitForPlugin() error { return err } -func (r *RestoreReaderImpl) positionReader(pos uint64, oid int) error { +func (r *RestoreReader) positionReader(pos uint64, oid int) error { switch r.readerType { case SEEKABLE: seekPosition, err := r.seekReader.Seek(int64(pos), io.SeekCurrent) @@ -103,7 +103,7 @@ func (r *RestoreReaderImpl) positionReader(pos uint64, oid int) error { return nil } -func (r *RestoreReaderImpl) copyData(num int64) (int64, error) { +func (r *RestoreReader) copyData(num int64) (int64, error) { var bytesRead int64 var err error switch r.readerType { @@ -115,7 +115,7 @@ func (r *RestoreReaderImpl) copyData(num int64) (int64, error) { return bytesRead, err } -func (r *RestoreReaderImpl) copyAllData() (int64, error) { +func (r *RestoreReader) copyAllData() (int64, error) { var bytesRead int64 var err error switch r.readerType { @@ -127,15 +127,15 @@ func (r *RestoreReaderImpl) copyAllData() (int64, error) { return bytesRead, err } -func (r *RestoreReaderImpl) getReaderType() ReaderType { +func (r *RestoreReader) getReaderType() ReaderType { return r.readerType } -func (r *RestoreReaderImpl) getFileHandle() *os.File { +func (r *RestoreReader) getFileHandle() *os.File { return r.fileHandle } -func closeAndDeletePipe(h Helper, tableOid int, batchNum int) { +func closeAndDeletePipe(h IHelper, tableOid int, batchNum int) { pipe := fmt.Sprintf("%s_%d_%d", *pipeFile, tableOid, batchNum) logInfo(fmt.Sprintf("Oid %d, Batch %d: Closing pipe %s", tableOid, batchNum, pipe)) errPipe := h.flushAndCloseRestoreWriter(pipe, tableOid) @@ -155,8 +155,8 @@ type oidWithBatch struct { batch int } -type RestoreHelper interface { - getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (RestoreReader, error) +type IRestoreHelper interface { + getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) checkForSkipFile(pipeFile string, tableOid int) bool } @@ -167,7 +167,13 @@ func (RestoreHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { return utils.FileExists(fmt.Sprintf("%s_skip_%d", pipeFile, tableOid)) } -func doRestoreAgentInternal(h Helper, rh RestoreHelper) error { +func doRestoreAgent() error { + helper := new(Helper) + restorer := new(RestoreHelperImpl) + return doRestoreAgentInternal(helper, restorer) +} + +func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { // We need to track various values separately per content for resize restore var segmentTOC map[int]*toc.SegmentTOC var tocEntries map[int]map[uint]toc.SegmentDataEntry @@ -178,7 +184,7 @@ func doRestoreAgentInternal(h Helper, rh RestoreHelper) error { var bytesRead int64 var lastError error - readers := make(map[int]RestoreReader) + readers := make(map[int]IRestoreReader) oidWithBatchList, err := h.getOidWithBatchListFromFile(*oidFile) if err != nil { @@ -442,12 +448,6 @@ func doRestoreAgentInternal(h Helper, rh RestoreHelper) error { return lastError } -func doRestoreAgent() error { - helper := new(HelperImpl) - restorer := new(RestoreHelperImpl) - return doRestoreAgentInternal(helper, restorer) -} - func constructSingleTableFilename(name string, contentToRestore int, oid int) string { name = strings.ReplaceAll(name, fmt.Sprintf("gpbackup_%d", *content), fmt.Sprintf("gpbackup_%d", contentToRestore)) nameParts := strings.Split(name, ".") @@ -467,13 +467,13 @@ func replaceContentInFilename(filename string, content int) string { return contentRE.ReplaceAllString(filename, fmt.Sprintf("gpbackup_%d_", content)) } -func (RestoreHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (RestoreReader, error) { +func (RestoreHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) { var readHandle io.Reader var seekHandle io.ReadSeeker var isSubset bool var err error = nil var pluginCmd *PluginCmd = nil - restoreReader := new(RestoreReaderImpl) + restoreReader := new(RestoreReader) if *pluginConfigFile != "" { pluginCmd, readHandle, isSubset, err = startRestorePluginCommand(fileToRead, objToc, oidList) From 64c5d30cbd67c9cbf653405a5c08208546a6c03e Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Wed, 4 Sep 2024 16:50:31 +0500 Subject: [PATCH 11/29] use closeFileHandle instead of getting handle --- helper/helper_test.go | 3 +-- helper/restore_helper.go | 8 ++++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index 2bbc4964a..167fbc235 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -40,8 +40,7 @@ func (r *RestoreReaderTestImpl) copyAllData() (int64, error) { return 1, nil } -func (r *RestoreReaderTestImpl) getFileHandle() *os.File { - return nil +func (r *RestoreReaderTestImpl) closeFileHandle() { } func (r *RestoreReaderTestImpl) getReaderType() ReaderType { diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 61c1d0451..6be495a49 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -48,7 +48,7 @@ type IRestoreReader interface { positionReader(pos uint64, oid int) error copyData(num int64) (int64, error) copyAllData() (int64, error) - getFileHandle() *os.File + closeFileHandle() getReaderType() ReaderType } @@ -131,8 +131,8 @@ func (r *RestoreReader) getReaderType() ReaderType { return r.readerType } -func (r *RestoreReader) getFileHandle() *os.File { - return r.fileHandle +func (r *RestoreReader) closeFileHandle() { + r.fileHandle.Close() } func closeAndDeletePipe(h IHelper, tableOid int, batchNum int) { @@ -313,7 +313,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { // Close file before it gets overwritten. Free up these // resources when the reader is not needed anymore. if reader, ok := readers[contentToRestore]; ok { - reader.getFileHandle().Close() + reader.closeFileHandle() } // We pre-create readers above for the sake of not re-opening SDF readers. For MDF we can't // re-use them but still having them in a map simplifies overall code flow. We repeatedly assign From fc23f5a0005016277c22978a8b531985196af1d2 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Wed, 4 Sep 2024 17:04:24 +0500 Subject: [PATCH 12/29] Add closeAndDeletePipe to the Helper interface --- helper/helper.go | 1 + helper/helper_test.go | 3 +++ helper/restore_helper.go | 6 +++--- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index 468fe2efa..ae9d9ea3b 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -216,6 +216,7 @@ type IHelper interface { getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) createPipe(pipe string) error flushAndCloseRestoreWriter(pipeName string, oid int) error + closeAndDeletePipe(tableOid int, batchNum int) } type Helper struct{} diff --git a/helper/helper_test.go b/helper/helper_test.go index 167fbc235..5184cb86c 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -83,6 +83,9 @@ func (h *RestoreMockHelperImpl) getCurStep() SkipFileTestStep { return h.expectedSteps[h.stepNo] } +func (h *RestoreMockHelperImpl) closeAndDeletePipe(tableOid int, batchNum int) { +} + func NewSkipFileTest(batches []oidWithBatch, steps []SkipFileTestStep) *RestoreMockHelperImpl { var ret = new(RestoreMockHelperImpl) ret.expectedOidBatch = batches diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 6be495a49..7db179b30 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -135,7 +135,7 @@ func (r *RestoreReader) closeFileHandle() { r.fileHandle.Close() } -func closeAndDeletePipe(h IHelper, tableOid int, batchNum int) { +func (h Helper) closeAndDeletePipe(tableOid int, batchNum int) { pipe := fmt.Sprintf("%s_%d_%d", *pipeFile, tableOid, batchNum) logInfo(fmt.Sprintf("Oid %d, Batch %d: Closing pipe %s", tableOid, batchNum, pipe)) errPipe := h.flushAndCloseRestoreWriter(pipe, tableOid) @@ -347,7 +347,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { for idx := 0; idx < *copyQueue; idx++ { batchToDelete := batchNum + idx if batchToDelete < batches { - closeAndDeletePipe(h, tableOid, batchToDelete) + h.closeAndDeletePipe(tableOid, batchToDelete) } } goto LoopEnd @@ -417,7 +417,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { logInfo(fmt.Sprintf("Oid %d, Batch %d: Copied %d bytes into the pipe", tableOid, batchNum, bytesRead)) LoopEnd: if tableOid != skipOid { - closeAndDeletePipe(h, tableOid, batchNum) + h.closeAndDeletePipe(tableOid, batchNum) } logVerbose(fmt.Sprintf("Oid %d, Batch %d: End batch restore", tableOid, batchNum)) From 0171ceeb822dc730d217200bd2c62117355e7242 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Mon, 9 Sep 2024 19:20:05 +0500 Subject: [PATCH 13/29] Rename RestoreHelperImpl to RestoreHelper --- helper/restore_helper.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 44ac3bc29..2b41e2536 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -161,15 +161,15 @@ type IRestoreHelper interface { checkForSkipFile(pipeFile string, tableOid int) bool } -type RestoreHelperImpl struct{} +type RestoreHelper struct{} -func (RestoreHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { +func (RestoreHelper) checkForSkipFile(pipeFile string, tableOid int) bool { return utils.FileExists(fmt.Sprintf("%s_skip_%d", pipeFile, tableOid)) } func doRestoreAgent() error { helper := new(Helper) - restorer := new(RestoreHelperImpl) + restorer := new(RestoreHelper) return doRestoreAgentInternal(helper, restorer) } @@ -467,7 +467,7 @@ func replaceContentInFilename(filename string, content int) string { return contentRE.ReplaceAllString(filename, fmt.Sprintf("gpbackup_%d_", content)) } -func (RestoreHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) { +func (RestoreHelper) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) { var readHandle io.Reader var seekHandle io.ReadSeeker var isSubset bool @@ -531,7 +531,7 @@ func (RestoreHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.Seg return restoreReader, err } -func (RestoreHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { +func (RestoreHelper) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { fileHandle, err := os.OpenFile(currentPipe, os.O_WRONLY|unix.O_NONBLOCK, os.ModeNamedPipe) if err != nil { // error logging handled by calling functions From b1fab12a972e86329fbde60e86a38b4774eb4492 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Mon, 16 Sep 2024 10:28:49 +0500 Subject: [PATCH 14/29] Do not save global variables for test and other cosmetic changes --- helper/helper_test.go | 86 ++----------------------------------------- 1 file changed, 4 insertions(+), 82 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index 5184cb86c..9efd42729 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -143,28 +143,6 @@ func (h *RestoreMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio return nil, nil, unix.ENXIO } -var ( - saveBackupAgent bool - saveCompressionLevel int - saveCompressionType string - saveContent int - saveDataFile string - saveOidFile string - saveOnErrorContinue bool - savePipeFile string - savePluginConfigFile string - savePrintVersion bool - saveRestoreAgent bool - saveTocFile string - saveIsFiltered bool - saveCopyQueue int - saveSingleDataFile bool - saveIsResizeRestore bool - saveOrigSize int - saveDestSize int - saveVerbosity int -) - var _ = Describe("helper tests", func() { var pluginConfig utils.PluginConfig var isSubset bool @@ -183,52 +161,10 @@ var _ = Describe("helper tests", func() { } BeforeEach(func() { - saveBackupAgent = *backupAgent - saveCompressionLevel = *compressionLevel - saveCompressionType = *compressionType - saveContent = *content - saveDataFile = *dataFile - saveOidFile = *oidFile - saveOnErrorContinue = *onErrorContinue - savePipeFile = *pipeFile - savePluginConfigFile = *pluginConfigFile - savePrintVersion = *printVersion - saveRestoreAgent = *restoreAgent - saveTocFile = *tocFile - saveIsFiltered = *isFiltered - saveCopyQueue = *copyQueue - saveSingleDataFile = *singleDataFile - saveIsResizeRestore = *isResizeRestore - saveOrigSize = *origSize - saveDestSize = *destSize - saveVerbosity = *verbosity - err := os.MkdirAll(testDir, 0777) Expect(err).ShouldNot(HaveOccurred()) }) - AfterEach(func() { - *backupAgent = saveBackupAgent - *compressionLevel = saveCompressionLevel - *compressionType = saveCompressionType - *content = saveContent - *dataFile = saveDataFile - *oidFile = saveOidFile - *onErrorContinue = saveOnErrorContinue - *pipeFile = savePipeFile - *pluginConfigFile = savePluginConfigFile - *printVersion = savePrintVersion - *restoreAgent = saveRestoreAgent - *tocFile = saveTocFile - *isFiltered = saveIsFiltered - *copyQueue = saveCopyQueue - *singleDataFile = saveSingleDataFile - *isResizeRestore = saveIsResizeRestore - *origSize = saveOrigSize - *destSize = saveDestSize - *verbosity = saveVerbosity - }) - Describe("Check subset flag", func() { It("when restore_subset is off, --on-error-continue is false, compression is not used", func() { pluginConfig.Options["restore_subset"] = "off" @@ -327,26 +263,12 @@ var _ = Describe("helper tests", func() { mockHelper := NewSkipFileTest(oidBatch, steps) - // Prepare the toc file + // Prepare and write the toc file testDir := "" //"/tmp/helper_test/20180101/20180101010101/" *tocFile = fmt.Sprintf("%stest_toc.yaml", testDir) - dataLength := 128*1024 + 1 - // Write custom TOC - customTOC := fmt.Sprintf(`dataentries: - 1: - startbyte: 0 - endbyte: 18 - 2: - startbyte: 18 - endbyte: %[1]d - 3: - startbyte: %[1]d - endbyte: %d -`, dataLength+18, dataLength+18+18) - fToc, _ := os.Create(*tocFile) - fToc.WriteString(customTOC) + writeTestTOC(*tocFile) defer func() { - os.Remove(*tocFile) + _ = os.Remove(*tocFile) }() *dataFile = "test_data.dat" @@ -466,6 +388,6 @@ func writeTestTOC(tocFile string) { `, dataLength+18, dataLength+18+18) fToc, err := os.Create(tocFile) Expect(err).ShouldNot(HaveOccurred()) + defer fToc.Close() fToc.WriteString(customTOC) - fToc.Close() } From b34c37f08b4246fdfc0a4229b717613a98e499dc Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Tue, 24 Sep 2024 23:47:44 +0500 Subject: [PATCH 15/29] Add test for plugin error conditions --- helper/helper_test.go | 180 +++++++++++++++++++++++++++++++-------- helper/restore_helper.go | 47 +++++++--- 2 files changed, 176 insertions(+), 51 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index 9efd42729..fbee2a071 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -16,54 +16,55 @@ import ( ) var ( - testDir = "/tmp/helper_test/20180101/20180101010101" - testTocFile = fmt.Sprintf("%s/test_toc.yaml", testDir) - examplePluginTestConfig = "/tmp/test_example_plugin_config.yaml" + testDir = "/tmp/helper_test/20180101/20180101010101" + testTocFile = fmt.Sprintf("%s/test_toc.yaml", testDir) ) -type RestoreReaderTestImpl struct{} +type restoreReaderTestImpl struct { + waitCount int +} -func (r *RestoreReaderTestImpl) waitForPlugin() error { - // No plugins yet, no errors to detect +func (r *restoreReaderTestImpl) waitForPlugin() error { + r.waitCount++ return nil } -func (r *RestoreReaderTestImpl) positionReader(pos uint64, oid int) error { +func (r *restoreReaderTestImpl) positionReader(pos uint64, oid int) error { return nil } -func (r *RestoreReaderTestImpl) copyData(num int64) (int64, error) { +func (r *restoreReaderTestImpl) copyData(num int64) (int64, error) { return 1, nil } -func (r *RestoreReaderTestImpl) copyAllData() (int64, error) { +func (r *restoreReaderTestImpl) copyAllData() (int64, error) { return 1, nil } -func (r *RestoreReaderTestImpl) closeFileHandle() { +func (r *restoreReaderTestImpl) closeFileHandle() { } -func (r *RestoreReaderTestImpl) getReaderType() ReaderType { +func (r *restoreReaderTestImpl) getReaderType() ReaderType { return "nil" } -type SkipFileTestStep struct { +type helperTestStep struct { getRestorePipeWriterArgExpect string getRestorePipeWriterResult bool checkSkipFileArgTableOid int checkSkipFileResult bool } -type RestoreMockHelperImpl struct { +type restoreMockHelperImpl struct { stepNo int expectedOidBatch []oidWithBatch - expectedSteps []SkipFileTestStep + expectedSteps []helperTestStep openedPipesMap map[string]string // Ginkgo matcher works over map value, will diplicate key here - restoreData *RestoreReaderTestImpl + restoreData *restoreReaderTestImpl } -func (h *RestoreMockHelperImpl) openedPipes() []string { +func (h *restoreMockHelperImpl) openedPipes() []string { if h.openedPipesMap == nil { h.openedPipesMap = make(map[string]string) @@ -78,36 +79,36 @@ func (h *RestoreMockHelperImpl) openedPipes() []string { return ret } -func (h *RestoreMockHelperImpl) getCurStep() SkipFileTestStep { +func (h *restoreMockHelperImpl) getCurStep() helperTestStep { Expect(h.stepNo).To(BeNumerically("<", len(h.expectedSteps))) return h.expectedSteps[h.stepNo] } -func (h *RestoreMockHelperImpl) closeAndDeletePipe(tableOid int, batchNum int) { +func (h *restoreMockHelperImpl) closeAndDeletePipe(tableOid int, batchNum int) { } -func NewSkipFileTest(batches []oidWithBatch, steps []SkipFileTestStep) *RestoreMockHelperImpl { - var ret = new(RestoreMockHelperImpl) +func newHelperTest(batches []oidWithBatch, steps []helperTestStep) *restoreMockHelperImpl { + var ret = new(restoreMockHelperImpl) ret.expectedOidBatch = batches ret.expectedSteps = steps ret.openedPipesMap = nil - ret.restoreData = &RestoreReaderTestImpl{} + ret.restoreData = &restoreReaderTestImpl{} return ret } -func (h *RestoreMockHelperImpl) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { +func (h *restoreMockHelperImpl) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { return h.expectedOidBatch, nil } -func (h *RestoreMockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { +func (h *restoreMockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { step := h.getCurStep() Expect(tableOid).To(Equal(step.checkSkipFileArgTableOid)) ret := h.getCurStep().checkSkipFileResult return ret } -func (h *RestoreMockHelperImpl) createPipe(pipe string) error { +func (h *restoreMockHelperImpl) createPipe(pipe string) error { // Check that pipe was not opened yet Expect(h.openedPipes()).ShouldNot(ContainElement(pipe)) @@ -115,21 +116,21 @@ func (h *RestoreMockHelperImpl) createPipe(pipe string) error { return nil } -func (h *RestoreMockHelperImpl) flushAndCloseRestoreWriter(pipeName string, oid int) error { +func (h *restoreMockHelperImpl) flushAndCloseRestoreWriter(pipeName string, oid int) error { // Check that we are closing pipe which is opened Expect(h.openedPipes()).To(ContainElement(pipeName)) delete(h.openedPipesMap, pipeName) return nil } -func (h *RestoreMockHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) { +func (h *restoreMockHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) { if h.restoreData != nil { return h.restoreData, nil } return nil, errors.New("getRestoreDataReader Not implemented") } -func (h *RestoreMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { +func (h *restoreMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { h.stepNo++ Expect(currentPipe).To(Equal(h.getCurStep().getRestorePipeWriterArgExpect)) @@ -143,6 +144,31 @@ func (h *RestoreMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio return nil, nil, unix.ENXIO } +type testPluginCmd struct { + hasProcess_ bool + error *string + waitCount int +} + +func (tp *testPluginCmd) hasProcess() bool { + return tp.hasProcess_ +} + +func (pt *testPluginCmd) Pid() int { + return 42 +} + +func (pt *testPluginCmd) Wait() error { + pt.waitCount += 1 + if pt.error == nil { + return nil + } + return errors.New(*pt.error) +} + +func (pt *testPluginCmd) errLog() { +} + var _ = Describe("helper tests", func() { var pluginConfig utils.PluginConfig var isSubset bool @@ -256,12 +282,12 @@ var _ = Describe("helper tests", func() { *singleDataFile = true oidBatch := []oidWithBatch{{oid: 1, batch: 1}} - steps := []SkipFileTestStep{ + steps := []helperTestStep{ {}, {getRestorePipeWriterArgExpect: "mock_1_1", getRestorePipeWriterResult: true, checkSkipFileArgTableOid: 1, checkSkipFileResult: false}, } - mockHelper := NewSkipFileTest(oidBatch, steps) + mockHelper := newHelperTest(oidBatch, steps) // Prepare and write the toc file testDir := "" //"/tmp/helper_test/20180101/20180101010101/" @@ -280,12 +306,12 @@ var _ = Describe("helper tests", func() { It("successfully restores using multiple data files when inputs are valid and no errors occur", func() { // Call the function under test oidBatch := []oidWithBatch{{oid: 1, batch: 1}} - steps := []SkipFileTestStep{ + steps := []helperTestStep{ {}, {getRestorePipeWriterArgExpect: "mock_1_1", getRestorePipeWriterResult: true, checkSkipFileArgTableOid: 1, checkSkipFileResult: false}, } - mockHelper := NewSkipFileTest(oidBatch, steps) + mockHelper := newHelperTest(oidBatch, steps) err := doRestoreAgentInternal(mockHelper, mockHelper) @@ -300,7 +326,7 @@ var _ = Describe("helper tests", func() { {200, 2}, } - expectedScenario := []SkipFileTestStep{ + expectedScenario := []helperTestStep{ {}, // placeholder as steps start from 1 {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called @@ -308,7 +334,7 @@ var _ = Describe("helper tests", func() { {"mock_200_2", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called } - helper := NewSkipFileTest(oidBatch, expectedScenario) + helper := newHelperTest(oidBatch, expectedScenario) err := doRestoreAgentInternal(helper, helper) Expect(err).To(BeNil()) @@ -325,7 +351,7 @@ var _ = Describe("helper tests", func() { {200, 2}, } - expectedScenario := []SkipFileTestStep{ + expectedScenario := []helperTestStep{ {}, // placeholder as steps start from 1 {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called @@ -333,7 +359,7 @@ var _ = Describe("helper tests", func() { {"mock_200_2", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called } - helper := NewSkipFileTest(oidBatch, expectedScenario) + helper := newHelperTest(oidBatch, expectedScenario) err := doRestoreAgentInternal(helper, helper) Expect(err).To(BeNil()) @@ -357,7 +383,7 @@ var _ = Describe("helper tests", func() { {200, 2}, } - expectedScenario := []SkipFileTestStep{ + expectedScenario := []helperTestStep{ {}, // placeholder as steps start from 1 {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called @@ -365,10 +391,90 @@ var _ = Describe("helper tests", func() { {"mock_200_2", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called } - helper := NewSkipFileTest(oidBatch, expectedScenario) + helper := newHelperTest(oidBatch, expectedScenario) err := doRestoreAgentInternal(helper, helper) Expect(err).To(BeNil()) }) + It("calls Wait in waitForPlugin doRestoreAgent for single data file", func() { + *singleDataFile = true + *isResizeRestore = false + *tocFile = testTocFile + + // Although pure concept would be to mock TOC file as well, to keep things simpler + // let's use real TOC file here + writeTestTOC(testTocFile) + defer func() { + _ = os.Remove(*tocFile) + }() + + oidBatch := []oidWithBatch{{100, 0}} + expectedScenario := []helperTestStep{{}, {"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although + helper := newHelperTest(oidBatch, expectedScenario) + + err := doRestoreAgentInternal(helper, helper) + Expect(err).ToNot(HaveOccurred()) + + // Check that plugin command's Wait was acually called and only once + Expect(helper.restoreData.waitCount).To(Equal(1)) + }) + It("calls waitForPlugin doRestoreAgent for resize and no single data file ", func() { + *singleDataFile = false + + oidBatch := []oidWithBatch{{100, 0}} + expectedScenario := []helperTestStep{{}, {"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although + + helper := newHelperTest(oidBatch, expectedScenario) + + err := doRestoreAgentInternal(helper, helper) + Expect(err).ToNot(HaveOccurred()) + + // Check that plugin command's Wait was acually called and only once + Expect(helper.restoreData.waitCount).To(Equal(1)) + }) + It("calls waitForPlugin doRestoreAgent for reduce cluster and no single data file ", func() { + *singleDataFile = false + *destSize, *origSize = *origSize, *destSize + + oidBatch := []oidWithBatch{{100, 0}} + expectedScenario := []helperTestStep{{}, {"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although + + helper := newHelperTest(oidBatch, expectedScenario) + + err := doRestoreAgentInternal(helper, helper) + Expect(err).ToNot(HaveOccurred()) + + // Check that plugin command's Wait was acually called and only once + Expect(helper.restoreData.waitCount).To(Equal(1)) + }) + }) + Describe("RestoreReader tests", func() { + It("waitForPlugin normal completion", func() { + test_cmd1 := testPluginCmd{hasProcess_: true} + test_reader := new(RestoreReader) + test_reader.pluginCmd = &test_cmd1 + + err := test_reader.waitForPlugin() + Expect(err).ToNot(HaveOccurred()) + Expect(test_cmd1.waitCount).To(Equal(1)) + + // Check that waitForPlugin do nothing when no cmd and/or no process + test_cmd2 := testPluginCmd{hasProcess_: false} + test_reader.pluginCmd = &test_cmd2 + err = test_reader.waitForPlugin() + Expect(err).ToNot(HaveOccurred()) + Expect(test_cmd2.waitCount).To(Equal(0)) + + }) + It("waitForPlugin error in Wait happened", func() { + msg := "Expected test error" + test_cmd1 := testPluginCmd{hasProcess_: true, error: &msg} + test_reader := new(RestoreReader) + test_reader.pluginCmd = &test_cmd1 + + err := test_reader.waitForPlugin() + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal(msg)) + }) }) }) diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 2b41e2536..0f42c39cf 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -56,7 +56,7 @@ type RestoreReader struct { fileHandle *os.File bufReader *bufio.Reader seekReader io.ReadSeeker - pluginCmd *PluginCmd + pluginCmd PluginCmd readerType ReaderType } @@ -64,19 +64,14 @@ type RestoreReader struct { // called on every reader that used a plugin as to not leave any zombies behind. func (r *RestoreReader) waitForPlugin() error { var err error - if r.pluginCmd != nil && r.pluginCmd.Process != nil { - logVerbose(fmt.Sprintf("Waiting for the plugin process (%d)", r.pluginCmd.Process.Pid)) + if r.pluginCmd != nil && r.pluginCmd.hasProcess() { + logVerbose(fmt.Sprintf("Waiting for the plugin process (%d)", r.pluginCmd.Pid())) err = r.pluginCmd.Wait() if err != nil { logError(fmt.Sprintf("Plugin process exited with an error: %s", err)) } // Log plugin's stderr as warnings. - errLog := strings.Trim(r.pluginCmd.errBuf.String(), "\x00") - if len(errLog) != 0 { - logWarn(fmt.Sprintf("Plugin log: %s", errLog)) - // Consume the entire buffer. - r.pluginCmd.errBuf.Next(r.pluginCmd.errBuf.Len()) - } + r.pluginCmd.errLog() } return err } @@ -472,7 +467,7 @@ func (RestoreHelper) getRestoreDataReader(fileToRead string, objToc *toc.Segment var seekHandle io.ReadSeeker var isSubset bool var err error = nil - var pluginCmd *PluginCmd = nil + var pluginCmd PluginCmd restoreReader := new(RestoreReader) if *pluginConfigFile != "" { @@ -504,7 +499,7 @@ func (RestoreHelper) getRestoreDataReader(fileToRead string, objToc *toc.Segment return nil, err } if pluginCmd != nil { - logVerbose(fmt.Sprintf("Started plugin process (%d)", pluginCmd.Process.Pid)) + logVerbose(fmt.Sprintf("Started plugin process (%d)", pluginCmd.Pid())) } // Set the underlying stream reader in restoreReader @@ -568,12 +563,36 @@ func getSubsetFlag(fileToRead string, pluginConfig *utils.PluginConfig) bool { // pluginCmd is needed to keep track of readable stderr and whether the command // has already been ended. -type PluginCmd struct { +type PluginCmd interface { + hasProcess() bool + Pid() int + Wait() error + errLog() +} + +type PluginCmdImpl struct { *exec.Cmd errBuf bytes.Buffer } -func startRestorePluginCommand(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (*PluginCmd, io.Reader, bool, error) { +func (p PluginCmdImpl) hasProcess() bool { + return p.Process != nil +} + +func (p PluginCmdImpl) Pid() int { + return p.Process.Pid +} + +func (p PluginCmdImpl) errLog() { + errLog := strings.Trim(p.errBuf.String(), "\x00") + if len(errLog) != 0 { + logWarn(fmt.Sprintf("Plugin log: %s", errLog)) + // Consume the entire buffer. + p.errBuf.Next(p.errBuf.Len()) + } +} + +func startRestorePluginCommand(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (PluginCmd, io.Reader, bool, error) { isSubset := false pluginConfig, err := utils.ReadPluginConfig(*pluginConfigFile) if err != nil { @@ -600,7 +619,7 @@ func startRestorePluginCommand(fileToRead string, objToc *toc.SegmentTOC, oidLis } logVerbose(cmdStr) - pluginCmd := &PluginCmd{} + pluginCmd := &PluginCmdImpl{} pluginCmd.Cmd = exec.Command("bash", "-c", cmdStr) pluginCmd.Stderr = &pluginCmd.errBuf From e45e70ae39787b29811945cd21e796f83714c491 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Wed, 2 Oct 2024 18:56:41 +0500 Subject: [PATCH 16/29] Refactoring suggested by the review --- helper/helper.go | 49 +------------------------------- helper/restore_helper.go | 60 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 54 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index ae9d9ea3b..616282c9e 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -1,7 +1,6 @@ package helper import ( - "bufio" "flag" "fmt" "os" @@ -26,8 +25,6 @@ var ( CleanupGroup *sync.WaitGroup version string wasTerminated bool - writeHandle *os.File - writer *bufio.Writer pipesMap map[string]bool ) @@ -213,10 +210,7 @@ func preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCo } type IHelper interface { - getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) createPipe(pipe string) error - flushAndCloseRestoreWriter(pipeName string, oid int) error - closeAndDeletePipe(tableOid int, batchNum int) } type Helper struct{} @@ -225,24 +219,6 @@ func (h Helper) createPipe(pipe string) error { return createPipe(pipe) } -func (h Helper) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { - oidStr, err := operating.System.ReadFile(oidFileName) - if err != nil { - logError(fmt.Sprintf("Error encountered reading oid batch list from file: %v", err)) - return nil, err - } - oidStrList := strings.Split(strings.TrimSpace(fmt.Sprintf("%s", oidStr)), "\n") - oidList := make([]oidWithBatch, len(oidStrList)) - for i, entry := range oidStrList { - oidWithBatchEntry := strings.Split(entry, ",") - oidNum, _ := strconv.Atoi(oidWithBatchEntry[0]) - batchNum, _ := strconv.Atoi(oidWithBatchEntry[1]) - - oidList[i] = oidWithBatch{oid: oidNum, batch: batchNum} - } - return oidList, nil -} - func getOidListFromFile(oidFileName string) ([]int, error) { oidStr, err := operating.System.ReadFile(oidFileName) if err != nil { @@ -258,29 +234,6 @@ func getOidListFromFile(oidFileName string) ([]int, error) { return oidList, nil } -func (h *Helper) flushAndCloseRestoreWriter(pipeName string, oid int) error { - if writer != nil { - writer.Write([]byte{}) // simulate writer connected in case of error - err := writer.Flush() - if err != nil { - logError("Oid %d: Failed to flush pipe %s", oid, pipeName) - return err - } - writer = nil - logVerbose("Oid %d: Successfully flushed pipe %s", oid, pipeName) - } - if writeHandle != nil { - err := writeHandle.Close() - if err != nil { - logError("Oid %d: Failed to close pipe handle", oid) - return err - } - writeHandle = nil - logVerbose("Oid %d: Successfully closed pipe handle", oid) - } - return nil -} - /* * Shared helper functions */ @@ -304,7 +257,7 @@ func DoCleanup() { logVerbose("Encountered error closing error file: %v", err) } } - err := new(Helper).flushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) + err := new(RestoreHelper).flushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) if err != nil { logVerbose("Encountered error during cleanup: %v", err) } diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 0f42c39cf..ef5a1863c 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -11,9 +11,11 @@ import ( "os/exec" "path" "regexp" + "strconv" "strings" "time" + "github.com/greenplum-db/gp-common-go-libs/operating" "github.com/greenplum-db/gpbackup/toc" "github.com/greenplum-db/gpbackup/utils" "github.com/klauspost/compress/zstd" @@ -33,7 +35,9 @@ const ( ) var ( - contentRE *regexp.Regexp + writeHandle *os.File + writer *bufio.Writer + contentRE *regexp.Regexp ) /* IRestoreReader interface to wrap the underlying reader. @@ -130,10 +134,10 @@ func (r *RestoreReader) closeFileHandle() { r.fileHandle.Close() } -func (h Helper) closeAndDeletePipe(tableOid int, batchNum int) { +func (rh RestoreHelper) closeAndDeletePipe(tableOid int, batchNum int) { pipe := fmt.Sprintf("%s_%d_%d", *pipeFile, tableOid, batchNum) logInfo(fmt.Sprintf("Oid %d, Batch %d: Closing pipe %s", tableOid, batchNum, pipe)) - err := h.flushAndCloseRestoreWriter(pipe, tableOid) + err := rh.flushAndCloseRestoreWriter(pipe, tableOid) if err != nil { logVerbose(fmt.Sprintf("Oid %d, Batch %d: Failed to flush and close pipe: %s", tableOid, batchNum, err)) } @@ -151,6 +155,9 @@ type oidWithBatch struct { } type IRestoreHelper interface { + getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) + flushAndCloseRestoreWriter(pipeName string, oid int) error + closeAndDeletePipe(tableOid int, batchNum int) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) checkForSkipFile(pipeFile string, tableOid int) bool @@ -181,7 +188,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { readers := make(map[int]IRestoreReader) - oidWithBatchList, err := h.getOidWithBatchListFromFile(*oidFile) + oidWithBatchList, err := rh.getOidWithBatchListFromFile(*oidFile) if err != nil { return err } @@ -342,7 +349,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { for idx := 0; idx < *copyQueue; idx++ { batchToDelete := batchNum + idx if batchToDelete < batches { - h.closeAndDeletePipe(tableOid, batchToDelete) + rh.closeAndDeletePipe(tableOid, batchToDelete) } } goto LoopEnd @@ -412,7 +419,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { logInfo(fmt.Sprintf("Oid %d, Batch %d: Copied %d bytes into the pipe", tableOid, batchNum, bytesRead)) LoopEnd: if tableOid != skipOid { - h.closeAndDeletePipe(tableOid, batchNum) + rh.closeAndDeletePipe(tableOid, batchNum) } logVerbose(fmt.Sprintf("Oid %d, Batch %d: End batch restore", tableOid, batchNum)) @@ -443,6 +450,29 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { return lastError } +func (rh *RestoreHelper) flushAndCloseRestoreWriter(pipeName string, oid int) error { + if writer != nil { + writer.Write([]byte{}) // simulate writer connected in case of error + err := writer.Flush() + if err != nil { + logError("Oid %d: Failed to flush pipe %s", oid, pipeName) + return err + } + writer = nil + logVerbose("Oid %d: Successfully flushed pipe %s", oid, pipeName) + } + if writeHandle != nil { + err := writeHandle.Close() + if err != nil { + logError("Oid %d: Failed to close pipe handle", oid) + return err + } + writeHandle = nil + logVerbose("Oid %d: Successfully closed pipe handle", oid) + } + return nil +} + func constructSingleTableFilename(name string, contentToRestore int, oid int) string { name = strings.ReplaceAll(name, fmt.Sprintf("gpbackup_%d", *content), fmt.Sprintf("gpbackup_%d", contentToRestore)) nameParts := strings.Split(name, ".") @@ -462,6 +492,24 @@ func replaceContentInFilename(filename string, content int) string { return contentRE.ReplaceAllString(filename, fmt.Sprintf("gpbackup_%d_", content)) } +func (h RestoreHelper) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { + oidStr, err := operating.System.ReadFile(oidFileName) + if err != nil { + logError(fmt.Sprintf("Error encountered reading oid batch list from file: %v", err)) + return nil, err + } + oidStrList := strings.Split(strings.TrimSpace(fmt.Sprintf("%s", oidStr)), "\n") + oidList := make([]oidWithBatch, len(oidStrList)) + for i, entry := range oidStrList { + oidWithBatchEntry := strings.Split(entry, ",") + oidNum, _ := strconv.Atoi(oidWithBatchEntry[0]) + batchNum, _ := strconv.Atoi(oidWithBatchEntry[1]) + + oidList[i] = oidWithBatch{oid: oidNum, batch: batchNum} + } + return oidList, nil +} + func (RestoreHelper) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) { var readHandle io.Reader var seekHandle io.ReadSeeker From 247d924cd49029eff328e004aa54353c9635c570 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Thu, 3 Oct 2024 10:34:49 +0500 Subject: [PATCH 17/29] Refactoring inspired by the code review --- helper/helper.go | 2 +- helper/restore_helper.go | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index 616282c9e..3f1a1cd68 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -257,7 +257,7 @@ func DoCleanup() { logVerbose("Encountered error closing error file: %v", err) } } - err := new(RestoreHelper).flushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) + err := Restorer.flushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) if err != nil { logVerbose("Encountered error during cleanup: %v", err) } diff --git a/helper/restore_helper.go b/helper/restore_helper.go index ef5a1863c..a81f38cbb 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -35,6 +35,8 @@ const ( ) var ( + Restorer IRestoreHelper = new(RestoreHelper) + writeHandle *os.File writer *bufio.Writer contentRE *regexp.Regexp @@ -171,8 +173,7 @@ func (RestoreHelper) checkForSkipFile(pipeFile string, tableOid int) bool { func doRestoreAgent() error { helper := new(Helper) - restorer := new(RestoreHelper) - return doRestoreAgentInternal(helper, restorer) + return doRestoreAgentInternal(helper, Restorer) } func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { From 6bba26642e32bbbc0194e0826fc09e9154aadbeb Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Thu, 10 Oct 2024 09:39:56 +0500 Subject: [PATCH 18/29] Changes according to review --- helper/helper.go | 2 +- helper/restore_helper.go | 23 +++++++++++------------ 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index 3f1a1cd68..6f091cf98 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -215,7 +215,7 @@ type IHelper interface { type Helper struct{} -func (h Helper) createPipe(pipe string) error { +func (Helper) createPipe(pipe string) error { return createPipe(pipe) } diff --git a/helper/restore_helper.go b/helper/restore_helper.go index a81f38cbb..28f219e04 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -62,7 +62,7 @@ type RestoreReader struct { fileHandle *os.File bufReader *bufio.Reader seekReader io.ReadSeeker - pluginCmd PluginCmd + pluginCmd IPluginCmd readerType ReaderType } @@ -172,8 +172,7 @@ func (RestoreHelper) checkForSkipFile(pipeFile string, tableOid int) bool { } func doRestoreAgent() error { - helper := new(Helper) - return doRestoreAgentInternal(helper, Restorer) + return doRestoreAgentInternal(new(Helper), Restorer) } func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { @@ -493,7 +492,7 @@ func replaceContentInFilename(filename string, content int) string { return contentRE.ReplaceAllString(filename, fmt.Sprintf("gpbackup_%d_", content)) } -func (h RestoreHelper) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { +func (RestoreHelper) getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) { oidStr, err := operating.System.ReadFile(oidFileName) if err != nil { logError(fmt.Sprintf("Error encountered reading oid batch list from file: %v", err)) @@ -516,7 +515,7 @@ func (RestoreHelper) getRestoreDataReader(fileToRead string, objToc *toc.Segment var seekHandle io.ReadSeeker var isSubset bool var err error = nil - var pluginCmd PluginCmd + var pluginCmd IPluginCmd restoreReader := new(RestoreReader) if *pluginConfigFile != "" { @@ -612,27 +611,27 @@ func getSubsetFlag(fileToRead string, pluginConfig *utils.PluginConfig) bool { // pluginCmd is needed to keep track of readable stderr and whether the command // has already been ended. -type PluginCmd interface { +type IPluginCmd interface { hasProcess() bool Pid() int Wait() error errLog() } -type PluginCmdImpl struct { +type PluginCmd struct { *exec.Cmd errBuf bytes.Buffer } -func (p PluginCmdImpl) hasProcess() bool { +func (p PluginCmd) hasProcess() bool { return p.Process != nil } -func (p PluginCmdImpl) Pid() int { +func (p PluginCmd) Pid() int { return p.Process.Pid } -func (p PluginCmdImpl) errLog() { +func (p PluginCmd) errLog() { errLog := strings.Trim(p.errBuf.String(), "\x00") if len(errLog) != 0 { logWarn(fmt.Sprintf("Plugin log: %s", errLog)) @@ -641,7 +640,7 @@ func (p PluginCmdImpl) errLog() { } } -func startRestorePluginCommand(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (PluginCmd, io.Reader, bool, error) { +func startRestorePluginCommand(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IPluginCmd, io.Reader, bool, error) { isSubset := false pluginConfig, err := utils.ReadPluginConfig(*pluginConfigFile) if err != nil { @@ -668,7 +667,7 @@ func startRestorePluginCommand(fileToRead string, objToc *toc.SegmentTOC, oidLis } logVerbose(cmdStr) - pluginCmd := &PluginCmdImpl{} + pluginCmd := &PluginCmd{} pluginCmd.Cmd = exec.Command("bash", "-c", cmdStr) pluginCmd.Stderr = &pluginCmd.errBuf From eaf99c6e5525682a0f83391804307837c7523a4c Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Fri, 11 Oct 2024 10:08:17 +0500 Subject: [PATCH 19/29] revert some changes to helper.go, make refactroring smaller --- helper/helper.go | 2 +- helper/restore_helper.go | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index 6f091cf98..b83a6e9fd 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -257,7 +257,7 @@ func DoCleanup() { logVerbose("Encountered error closing error file: %v", err) } } - err := Restorer.flushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) + err := FlushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) if err != nil { logVerbose("Encountered error during cleanup: %v", err) } diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 28f219e04..d8ce62281 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -35,8 +35,6 @@ const ( ) var ( - Restorer IRestoreHelper = new(RestoreHelper) - writeHandle *os.File writer *bufio.Writer contentRE *regexp.Regexp @@ -172,7 +170,7 @@ func (RestoreHelper) checkForSkipFile(pipeFile string, tableOid int) bool { } func doRestoreAgent() error { - return doRestoreAgentInternal(new(Helper), Restorer) + return doRestoreAgentInternal(new(Helper), new(RestoreHelper)) } func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { @@ -450,7 +448,11 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { return lastError } -func (rh *RestoreHelper) flushAndCloseRestoreWriter(pipeName string, oid int) error { +func (RestoreHelper) flushAndCloseRestoreWriter(pipeName string, oid int) error { + return FlushAndCloseRestoreWriter(pipeName, oid) +} + +func FlushAndCloseRestoreWriter(pipeName string, oid int) error { if writer != nil { writer.Write([]byte{}) // simulate writer connected in case of error err := writer.Flush() From 173fbbaf73a83aad8b4ed45b4569b5169249dcb6 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Wed, 16 Oct 2024 00:03:01 +0500 Subject: [PATCH 20/29] Refactoring inspired by the code review --- helper/helper_test.go | 85 ++++++++++++++++++++++++------------------- 1 file changed, 48 insertions(+), 37 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index fbee2a071..0aed59b27 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -49,39 +49,52 @@ func (r *restoreReaderTestImpl) getReaderType() ReaderType { } type helperTestStep struct { - getRestorePipeWriterArgExpect string - getRestorePipeWriterResult bool - checkSkipFileArgTableOid int - checkSkipFileResult bool + restorePipeWriterArgExpect string + restorePipeWriterResult bool + skipFileArgTableOid int + skipFileResult bool } type restoreMockHelperImpl struct { - stepNo int + currentStep int + started bool expectedOidBatch []oidWithBatch expectedSteps []helperTestStep - openedPipesMap map[string]string // Ginkgo matcher works over map value, will diplicate key here - restoreData *restoreReaderTestImpl + pipesMap map[string]string // Ginkgo matcher works over map value, will diplicate key here + restoreData *restoreReaderTestImpl } func (h *restoreMockHelperImpl) openedPipes() []string { - if h.openedPipesMap == nil { - h.openedPipesMap = make(map[string]string) + if h.pipesMap == nil { + h.pipesMap = make(map[string]string) for k := range pipesMap { - h.openedPipesMap[k] = k + h.pipesMap[k] = k } } - ret := make([]string, 0, len(h.openedPipesMap)) - for k := range h.openedPipesMap { + ret := make([]string, 0, len(h.pipesMap)) + for k := range h.pipesMap { ret = append(ret, k) } return ret } +func (h *restoreMockHelperImpl) makeStep() helperTestStep { + if !h.started { + h.started = true + } else { + h.currentStep++ + } + + Expect(h.currentStep).To(BeNumerically("<", len(h.expectedSteps))) + ret := h.expectedSteps[h.currentStep] + return ret +} + func (h *restoreMockHelperImpl) getCurStep() helperTestStep { - Expect(h.stepNo).To(BeNumerically("<", len(h.expectedSteps))) - return h.expectedSteps[h.stepNo] + Expect(h.currentStep).To(BeNumerically("<", len(h.expectedSteps))) + return h.expectedSteps[h.currentStep] } func (h *restoreMockHelperImpl) closeAndDeletePipe(tableOid int, batchNum int) { @@ -91,7 +104,7 @@ func newHelperTest(batches []oidWithBatch, steps []helperTestStep) *restoreMockH var ret = new(restoreMockHelperImpl) ret.expectedOidBatch = batches ret.expectedSteps = steps - ret.openedPipesMap = nil + ret.pipesMap = nil ret.restoreData = &restoreReaderTestImpl{} return ret @@ -103,8 +116,8 @@ func (h *restoreMockHelperImpl) getOidWithBatchListFromFile(oidFileName string) func (h *restoreMockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) bool { step := h.getCurStep() - Expect(tableOid).To(Equal(step.checkSkipFileArgTableOid)) - ret := h.getCurStep().checkSkipFileResult + Expect(tableOid).To(Equal(step.skipFileArgTableOid)) + ret := step.skipFileResult return ret } @@ -112,32 +125,30 @@ func (h *restoreMockHelperImpl) createPipe(pipe string) error { // Check that pipe was not opened yet Expect(h.openedPipes()).ShouldNot(ContainElement(pipe)) - h.openedPipesMap[pipe] = pipe + h.pipesMap[pipe] = pipe return nil } func (h *restoreMockHelperImpl) flushAndCloseRestoreWriter(pipeName string, oid int) error { // Check that we are closing pipe which is opened Expect(h.openedPipes()).To(ContainElement(pipeName)) - delete(h.openedPipesMap, pipeName) + delete(h.pipesMap, pipeName) return nil } func (h *restoreMockHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) { - if h.restoreData != nil { - return h.restoreData, nil - } - return nil, errors.New("getRestoreDataReader Not implemented") + Expect(h.restoreData).ToNot(BeNil()) + return h.restoreData, nil } func (h *restoreMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) { - h.stepNo++ - Expect(currentPipe).To(Equal(h.getCurStep().getRestorePipeWriterArgExpect)) + step := h.makeStep() + Expect(currentPipe).To(Equal(step.restorePipeWriterArgExpect)) // The pipe should be created before Expect(h.openedPipes()).Should(ContainElement(currentPipe)) - if h.getCurStep().getRestorePipeWriterResult { + if step.restorePipeWriterResult { var writer bufio.Writer return &writer, nil, nil } @@ -283,14 +294,13 @@ var _ = Describe("helper tests", func() { oidBatch := []oidWithBatch{{oid: 1, batch: 1}} steps := []helperTestStep{ - {}, - {getRestorePipeWriterArgExpect: "mock_1_1", getRestorePipeWriterResult: true, checkSkipFileArgTableOid: 1, checkSkipFileResult: false}, + {restorePipeWriterArgExpect: "mock_1_1", restorePipeWriterResult: true, skipFileArgTableOid: 1, skipFileResult: false}, } mockHelper := newHelperTest(oidBatch, steps) // Prepare and write the toc file - testDir := "" //"/tmp/helper_test/20180101/20180101010101/" + testDir := "" // Use local directory for the TOC file instead of default onr *tocFile = fmt.Sprintf("%stest_toc.yaml", testDir) writeTestTOC(*tocFile) defer func() { @@ -307,8 +317,12 @@ var _ = Describe("helper tests", func() { // Call the function under test oidBatch := []oidWithBatch{{oid: 1, batch: 1}} steps := []helperTestStep{ - {}, - {getRestorePipeWriterArgExpect: "mock_1_1", getRestorePipeWriterResult: true, checkSkipFileArgTableOid: 1, checkSkipFileResult: false}, + { + restorePipeWriterArgExpect: "mock_1_1", + restorePipeWriterResult: true, + skipFileArgTableOid: 1, + skipFileResult: false, + }, } mockHelper := newHelperTest(oidBatch, steps) @@ -327,7 +341,6 @@ var _ = Describe("helper tests", func() { } expectedScenario := []helperTestStep{ - {}, // placeholder as steps start from 1 {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called {"mock_200_1", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists @@ -352,7 +365,6 @@ var _ = Describe("helper tests", func() { } expectedScenario := []helperTestStep{ - {}, // placeholder as steps start from 1 {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called {"mock_200_1", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists @@ -384,7 +396,6 @@ var _ = Describe("helper tests", func() { } expectedScenario := []helperTestStep{ - {}, // placeholder as steps start from 1 {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called {"mock_200_1", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists @@ -408,7 +419,7 @@ var _ = Describe("helper tests", func() { }() oidBatch := []oidWithBatch{{100, 0}} - expectedScenario := []helperTestStep{{}, {"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although + expectedScenario := []helperTestStep{{"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although helper := newHelperTest(oidBatch, expectedScenario) err := doRestoreAgentInternal(helper, helper) @@ -421,7 +432,7 @@ var _ = Describe("helper tests", func() { *singleDataFile = false oidBatch := []oidWithBatch{{100, 0}} - expectedScenario := []helperTestStep{{}, {"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although + expectedScenario := []helperTestStep{{"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although helper := newHelperTest(oidBatch, expectedScenario) @@ -436,7 +447,7 @@ var _ = Describe("helper tests", func() { *destSize, *origSize = *origSize, *destSize oidBatch := []oidWithBatch{{100, 0}} - expectedScenario := []helperTestStep{{}, {"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although + expectedScenario := []helperTestStep{{"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although helper := newHelperTest(oidBatch, expectedScenario) From 696c2b2ee1669d38f983251552f0f2a84c270017 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Mon, 21 Oct 2024 09:26:49 +0500 Subject: [PATCH 21/29] Regactoring for the code review. --- helper/helper_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index 0aed59b27..a5de997f6 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -429,7 +429,7 @@ var _ = Describe("helper tests", func() { Expect(helper.restoreData.waitCount).To(Equal(1)) }) It("calls waitForPlugin doRestoreAgent for resize and no single data file ", func() { - *singleDataFile = false + Expect(*singleDataFile).To(Equal(false)) oidBatch := []oidWithBatch{{100, 0}} expectedScenario := []helperTestStep{{"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although @@ -443,7 +443,7 @@ var _ = Describe("helper tests", func() { Expect(helper.restoreData.waitCount).To(Equal(1)) }) It("calls waitForPlugin doRestoreAgent for reduce cluster and no single data file ", func() { - *singleDataFile = false + Expect(*singleDataFile).To(Equal(false)) *destSize, *origSize = *origSize, *destSize oidBatch := []oidWithBatch{{100, 0}} From dd8a4daf7dcbad1573daa6a01c82e3eb74bac0f6 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Tue, 22 Oct 2024 09:37:57 +0500 Subject: [PATCH 22/29] Move comment into test description, simplify pipes list --- helper/helper_test.go | 46 +++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index a5de997f6..517b64600 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -53,6 +53,7 @@ type helperTestStep struct { restorePipeWriterResult bool skipFileArgTableOid int skipFileResult bool + comment string } type restoreMockHelperImpl struct { @@ -61,16 +62,16 @@ type restoreMockHelperImpl struct { expectedOidBatch []oidWithBatch expectedSteps []helperTestStep - pipesMap map[string]string // Ginkgo matcher works over map value, will diplicate key here + pipesMap map[string]bool restoreData *restoreReaderTestImpl } func (h *restoreMockHelperImpl) openedPipes() []string { if h.pipesMap == nil { - h.pipesMap = make(map[string]string) + h.pipesMap = make(map[string]bool) for k := range pipesMap { - h.pipesMap[k] = k + h.pipesMap[k] = true } } ret := make([]string, 0, len(h.pipesMap)) @@ -89,6 +90,7 @@ func (h *restoreMockHelperImpl) makeStep() helperTestStep { Expect(h.currentStep).To(BeNumerically("<", len(h.expectedSteps))) ret := h.expectedSteps[h.currentStep] + fmt.Printf("Step: %s", ret.comment) return ret } @@ -125,7 +127,7 @@ func (h *restoreMockHelperImpl) createPipe(pipe string) error { // Check that pipe was not opened yet Expect(h.openedPipes()).ShouldNot(ContainElement(pipe)) - h.pipesMap[pipe] = pipe + h.pipesMap[pipe] = true return nil } @@ -341,10 +343,10 @@ var _ = Describe("helper tests", func() { } expectedScenario := []helperTestStep{ - {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called - {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called - {"mock_200_1", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists - {"mock_200_2", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called + {"mock_100_0", true, -1, false, "Can open pipe for table 100, check_skip_file shall not be called"}, + {"mock_200_0", true, -1, false, "Can open pipe for table 200, check_skip_file shall not be called"}, + {"mock_200_1", false, 200, true, "Can not open pipe for table 200, check_skip_file shall called, skip file exists"}, + {"mock_200_2", true, -1, false, "Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called"}, } helper := newHelperTest(oidBatch, expectedScenario) @@ -365,10 +367,10 @@ var _ = Describe("helper tests", func() { } expectedScenario := []helperTestStep{ - {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called - {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called - {"mock_200_1", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists - {"mock_200_2", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called + {"mock_100_0", true, -1, false, "Can open pipe for table 100, check_skip_file shall not be called"}, + {"mock_200_0", true, -1, false, "Can open pipe for table 200, check_skip_file shall not be called"}, + {"mock_200_1", false, 200, true, "Can not open pipe for table 200, check_skip_file shall called, skip file exists"}, + {"mock_200_2", true, -1, false, "Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called"}, } helper := newHelperTest(oidBatch, expectedScenario) @@ -396,10 +398,10 @@ var _ = Describe("helper tests", func() { } expectedScenario := []helperTestStep{ - {"mock_100_0", true, -1, false}, // Can open pipe for table 100, check_skip_file shall not be called - {"mock_200_0", true, -1, false}, // Can open pipe for table 200, check_skip_file shall not be called - {"mock_200_1", false, 200, true}, // Can not open pipe for table 200, check_skip_file shall called, skip file exists - {"mock_200_2", true, -1, false}, // Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called + {"mock_100_0", true, -1, false, "Can open pipe for table 100, check_skip_file shall not be called"}, + {"mock_200_0", true, -1, false, "Can open pipe for table 200, check_skip_file shall not be called"}, + {"mock_200_1", false, 200, true, "Can not open pipe for table 200, check_skip_file shall called, skip file exists"}, + {"mock_200_2", true, -1, false, "Went to the next batch, Can open pipe for table 200, check_skip_file shall not be called"}, } helper := newHelperTest(oidBatch, expectedScenario) @@ -419,7 +421,9 @@ var _ = Describe("helper tests", func() { }() oidBatch := []oidWithBatch{{100, 0}} - expectedScenario := []helperTestStep{{"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although + expectedScenario := []helperTestStep{ + {"mock_100_0", true, -1, false, "Some pipe shall be created, out of interest for this test although"}, + } helper := newHelperTest(oidBatch, expectedScenario) err := doRestoreAgentInternal(helper, helper) @@ -432,7 +436,9 @@ var _ = Describe("helper tests", func() { Expect(*singleDataFile).To(Equal(false)) oidBatch := []oidWithBatch{{100, 0}} - expectedScenario := []helperTestStep{{"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although + expectedScenario := []helperTestStep{ + {"mock_100_0", true, -1, false, "Some pipe shall be created, out of interest for this test although"}, + } helper := newHelperTest(oidBatch, expectedScenario) @@ -447,7 +453,9 @@ var _ = Describe("helper tests", func() { *destSize, *origSize = *origSize, *destSize oidBatch := []oidWithBatch{{100, 0}} - expectedScenario := []helperTestStep{{"mock_100_0", true, -1, false}} // Some pipe shall be created, out of interest for this test although + expectedScenario := []helperTestStep{ + {"mock_100_0", true, -1, false, "Some pipe shall be created, out of interest for this test although"}, + } helper := newHelperTest(oidBatch, expectedScenario) From fb011fa98a74b9a61f2c3b2695914901c72dad13 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Wed, 23 Oct 2024 19:31:37 +0500 Subject: [PATCH 23/29] Use map search for opened pipes, use mocked struct to store preloaded pipe --- helper/helper.go | 3 ++- helper/helper_test.go | 34 +++++++++++++++++++++------------- helper/restore_helper.go | 2 +- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index b83a6e9fd..955d38a08 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -202,7 +202,7 @@ func preloadCreatedPipesForBackup(oidList []int, queuedPipeCount int) { } } -func preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCount int) { +func (h *Helper) preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCount int) { for i := 0; i < queuedPipeCount; i++ { pipeName := fmt.Sprintf("%s_%d_%d", *pipeFile, oidWithBatchList[i].oid, oidWithBatchList[i].batch) pipesMap[pipeName] = true @@ -211,6 +211,7 @@ func preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCo type IHelper interface { createPipe(pipe string) error + preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCount int) } type Helper struct{} diff --git a/helper/helper_test.go b/helper/helper_test.go index 517b64600..8ebbdfedb 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -66,17 +66,13 @@ type restoreMockHelperImpl struct { restoreData *restoreReaderTestImpl } -func (h *restoreMockHelperImpl) openedPipes() []string { - if h.pipesMap == nil { - h.pipesMap = make(map[string]bool) +func (h *restoreMockHelperImpl) isPipeOpened(pipe string) bool { + Expect(h.pipesMap).ToNot(BeNil()) - for k := range pipesMap { - h.pipesMap[k] = true - } - } - ret := make([]string, 0, len(h.pipesMap)) - for k := range h.pipesMap { - ret = append(ret, k) + ret, ok := h.pipesMap[pipe] + + if !ok { + return false } return ret } @@ -125,15 +121,27 @@ func (h *restoreMockHelperImpl) checkForSkipFile(pipeFile string, tableOid int) func (h *restoreMockHelperImpl) createPipe(pipe string) error { // Check that pipe was not opened yet - Expect(h.openedPipes()).ShouldNot(ContainElement(pipe)) + Expect(h.isPipeOpened(pipe)).To(Equal(false)) h.pipesMap[pipe] = true return nil } +func (h *restoreMockHelperImpl) preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCount int) { + + Expect(h.pipesMap).To(BeNil()) + + h.pipesMap = make(map[string]bool) + + for i := 0; i < queuedPipeCount; i++ { + pipeName := fmt.Sprintf("%s_%d_%d", *pipeFile, oidWithBatchList[i].oid, oidWithBatchList[i].batch) + h.pipesMap[pipeName] = true + } +} + func (h *restoreMockHelperImpl) flushAndCloseRestoreWriter(pipeName string, oid int) error { // Check that we are closing pipe which is opened - Expect(h.openedPipes()).To(ContainElement(pipeName)) + Expect(h.isPipeOpened(pipeName)).To(Equal(true)) delete(h.pipesMap, pipeName) return nil } @@ -148,7 +156,7 @@ func (h *restoreMockHelperImpl) getRestorePipeWriter(currentPipe string) (*bufio Expect(currentPipe).To(Equal(step.restorePipeWriterArgExpect)) // The pipe should be created before - Expect(h.openedPipes()).Should(ContainElement(currentPipe)) + Expect(h.isPipeOpened(currentPipe)).To(Equal(true)) if step.restorePipeWriterResult { var writer bufio.Writer diff --git a/helper/restore_helper.go b/helper/restore_helper.go index d8ce62281..80af785ba 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -259,7 +259,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { } } - preloadCreatedPipesForRestore(oidWithBatchList, *copyQueue) + h.preloadCreatedPipesForRestore(oidWithBatchList, *copyQueue) var currentPipe string From 915c9595e4a3a74ad106128cdae216bbace19563 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Thu, 24 Oct 2024 09:33:05 +0500 Subject: [PATCH 24/29] Use bool zero value as pipesMap lookup result Co-authored-by: Roman Eskin --- helper/helper_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index 8ebbdfedb..762ac72cf 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -69,12 +69,7 @@ type restoreMockHelperImpl struct { func (h *restoreMockHelperImpl) isPipeOpened(pipe string) bool { Expect(h.pipesMap).ToNot(BeNil()) - ret, ok := h.pipesMap[pipe] - - if !ok { - return false - } - return ret + return h.pipesMap[pipe] } func (h *restoreMockHelperImpl) makeStep() helperTestStep { From 898f5faf30708dc6f8381be684a869f835939c50 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Thu, 24 Oct 2024 09:42:24 +0500 Subject: [PATCH 25/29] Make code more readable --- helper/helper_test.go | 3 --- helper/restore_helper.go | 20 ++++++++++---------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index 762ac72cf..adf6b1c8a 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -123,7 +123,6 @@ func (h *restoreMockHelperImpl) createPipe(pipe string) error { } func (h *restoreMockHelperImpl) preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCount int) { - Expect(h.pipesMap).To(BeNil()) h.pipesMap = make(map[string]bool) @@ -379,7 +378,6 @@ var _ = Describe("helper tests", func() { helper := newHelperTest(oidBatch, expectedScenario) err := doRestoreAgentInternal(helper, helper) Expect(err).To(BeNil()) - }) It("skips batches if skip file is discovered with single datafile", func() { *singleDataFile = true @@ -485,7 +483,6 @@ var _ = Describe("helper tests", func() { err = test_reader.waitForPlugin() Expect(err).ToNot(HaveOccurred()) Expect(test_cmd2.waitCount).To(Equal(0)) - }) It("waitForPlugin error in Wait happened", func() { msg := "Expected test error" diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 80af785ba..f3f9e4826 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -173,7 +173,7 @@ func doRestoreAgent() error { return doRestoreAgentInternal(new(Helper), new(RestoreHelper)) } -func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { +func doRestoreAgentInternal(helper IHelper, restoreHelper IRestoreHelper) error { // We need to track various values separately per content for resize restore var segmentTOC map[int]*toc.SegmentTOC var tocEntries map[int]map[uint]toc.SegmentDataEntry @@ -186,7 +186,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { readers := make(map[int]IRestoreReader) - oidWithBatchList, err := rh.getOidWithBatchListFromFile(*oidFile) + oidWithBatchList, err := restoreHelper.getOidWithBatchListFromFile(*oidFile) if err != nil { return err } @@ -234,7 +234,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { tocEntries[contentToRestore] = segmentTOC[contentToRestore].DataEntries filename := replaceContentInFilename(*dataFile, contentToRestore) - readers[contentToRestore], err = rh.getRestoreDataReader(filename, segmentTOC[contentToRestore], oidList) + readers[contentToRestore], err = restoreHelper.getRestoreDataReader(filename, segmentTOC[contentToRestore], oidList) if readers[contentToRestore] != nil { // NOTE: If we reach here with batches > 1, there will be // *origSize / *destSize (N old segments / N new segments) @@ -259,7 +259,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { } } - h.preloadCreatedPipesForRestore(oidWithBatchList, *copyQueue) + helper.preloadCreatedPipesForRestore(oidWithBatchList, *copyQueue) var currentPipe string @@ -285,7 +285,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { nextBatchNum := nextOidWithBatch.batch nextPipeToCreate := fmt.Sprintf("%s_%d_%d", *pipeFile, nextOid, nextBatchNum) logVerbose(fmt.Sprintf("Oid %d, Batch %d: Creating pipe %s\n", nextOid, nextBatchNum, nextPipeToCreate)) - err := h.createPipe(nextPipeToCreate) + err := helper.createPipe(nextPipeToCreate) if err != nil { logError(fmt.Sprintf("Oid %d, Batch %d: Failed to create pipe %s\n", nextOid, nextBatchNum, nextPipeToCreate)) // In the case this error is hit it means we have lost the @@ -318,7 +318,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { // We pre-create readers above for the sake of not re-opening SDF readers. For MDF we can't // re-use them but still having them in a map simplifies overall code flow. We repeatedly assign // to a map entry here intentionally. - readers[contentToRestore], err = rh.getRestoreDataReader(filename, nil, nil) + readers[contentToRestore], err = restoreHelper.getRestoreDataReader(filename, nil, nil) if err != nil { logError(fmt.Sprintf("Oid: %d, Batch %d: Error encountered getting restore data reader: %v", tableOid, batchNum, err)) return err @@ -328,7 +328,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { logInfo(fmt.Sprintf("Oid %d, Batch %d: Opening pipe %s", tableOid, batchNum, currentPipe)) for { - writer, writeHandle, err = rh.getRestorePipeWriter(currentPipe) + writer, writeHandle, err = restoreHelper.getRestorePipeWriter(currentPipe) if err != nil { if errors.Is(err, unix.ENXIO) { // COPY (the pipe reader) has not tried to access the pipe yet so our restore_helper @@ -339,7 +339,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { // might be good to have a GPDB version check here. However, the restore helper should // not contain a database connection so the version should be passed through the helper // invocation from gprestore (e.g. create a --db-version flag option). - if *onErrorContinue && rh.checkForSkipFile(*pipeFile, tableOid) { + if *onErrorContinue && restoreHelper.checkForSkipFile(*pipeFile, tableOid) { logWarn(fmt.Sprintf("Oid %d, Batch %d: Skip file discovered, skipping this relation.", tableOid, batchNum)) err = nil skipOid = tableOid @@ -347,7 +347,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { for idx := 0; idx < *copyQueue; idx++ { batchToDelete := batchNum + idx if batchToDelete < batches { - rh.closeAndDeletePipe(tableOid, batchToDelete) + restoreHelper.closeAndDeletePipe(tableOid, batchToDelete) } } goto LoopEnd @@ -417,7 +417,7 @@ func doRestoreAgentInternal(h IHelper, rh IRestoreHelper) error { logInfo(fmt.Sprintf("Oid %d, Batch %d: Copied %d bytes into the pipe", tableOid, batchNum, bytesRead)) LoopEnd: if tableOid != skipOid { - rh.closeAndDeletePipe(tableOid, batchNum) + restoreHelper.closeAndDeletePipe(tableOid, batchNum) } logVerbose(fmt.Sprintf("Oid %d, Batch %d: End batch restore", tableOid, batchNum)) From 08edf940674951577d64dcbb0d1bc51134840d25 Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Thu, 24 Oct 2024 15:51:03 +0500 Subject: [PATCH 26/29] Simplify test after code review --- helper/helper.go | 18 ------------------ helper/helper_test.go | 16 ++++++++-------- helper/restore_helper.go | 23 +++++++++++++++++++---- 3 files changed, 27 insertions(+), 30 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index 955d38a08..af2d3ca04 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -202,24 +202,6 @@ func preloadCreatedPipesForBackup(oidList []int, queuedPipeCount int) { } } -func (h *Helper) preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCount int) { - for i := 0; i < queuedPipeCount; i++ { - pipeName := fmt.Sprintf("%s_%d_%d", *pipeFile, oidWithBatchList[i].oid, oidWithBatchList[i].batch) - pipesMap[pipeName] = true - } -} - -type IHelper interface { - createPipe(pipe string) error - preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCount int) -} - -type Helper struct{} - -func (Helper) createPipe(pipe string) error { - return createPipe(pipe) -} - func getOidListFromFile(oidFileName string) ([]int, error) { oidStr, err := operating.System.ReadFile(oidFileName) if err != nil { diff --git a/helper/helper_test.go b/helper/helper_test.go index adf6b1c8a..d85ffba6f 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -313,7 +313,7 @@ var _ = Describe("helper tests", func() { *dataFile = "test_data.dat" // Call the function under test - err := doRestoreAgentInternal(mockHelper, mockHelper) + err := doRestoreAgentInternal(mockHelper) Expect(err).ToNot(HaveOccurred()) }) @@ -331,7 +331,7 @@ var _ = Describe("helper tests", func() { mockHelper := newHelperTest(oidBatch, steps) - err := doRestoreAgentInternal(mockHelper, mockHelper) + err := doRestoreAgentInternal(mockHelper) Expect(err).ToNot(HaveOccurred()) }) @@ -353,7 +353,7 @@ var _ = Describe("helper tests", func() { helper := newHelperTest(oidBatch, expectedScenario) - err := doRestoreAgentInternal(helper, helper) + err := doRestoreAgentInternal(helper) Expect(err).To(BeNil()) }) It("skips batches if skip file is discovered with resize restore", func() { @@ -376,7 +376,7 @@ var _ = Describe("helper tests", func() { } helper := newHelperTest(oidBatch, expectedScenario) - err := doRestoreAgentInternal(helper, helper) + err := doRestoreAgentInternal(helper) Expect(err).To(BeNil()) }) It("skips batches if skip file is discovered with single datafile", func() { @@ -406,7 +406,7 @@ var _ = Describe("helper tests", func() { } helper := newHelperTest(oidBatch, expectedScenario) - err := doRestoreAgentInternal(helper, helper) + err := doRestoreAgentInternal(helper) Expect(err).To(BeNil()) }) It("calls Wait in waitForPlugin doRestoreAgent for single data file", func() { @@ -427,7 +427,7 @@ var _ = Describe("helper tests", func() { } helper := newHelperTest(oidBatch, expectedScenario) - err := doRestoreAgentInternal(helper, helper) + err := doRestoreAgentInternal(helper) Expect(err).ToNot(HaveOccurred()) // Check that plugin command's Wait was acually called and only once @@ -443,7 +443,7 @@ var _ = Describe("helper tests", func() { helper := newHelperTest(oidBatch, expectedScenario) - err := doRestoreAgentInternal(helper, helper) + err := doRestoreAgentInternal(helper) Expect(err).ToNot(HaveOccurred()) // Check that plugin command's Wait was acually called and only once @@ -460,7 +460,7 @@ var _ = Describe("helper tests", func() { helper := newHelperTest(oidBatch, expectedScenario) - err := doRestoreAgentInternal(helper, helper) + err := doRestoreAgentInternal(helper) Expect(err).ToNot(HaveOccurred()) // Check that plugin command's Wait was acually called and only once diff --git a/helper/restore_helper.go b/helper/restore_helper.go index f3f9e4826..6bca742d1 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -134,6 +134,10 @@ func (r *RestoreReader) closeFileHandle() { r.fileHandle.Close() } +func (RestoreHelper) createPipe(pipe string) error { + return createPipe(pipe) +} + func (rh RestoreHelper) closeAndDeletePipe(tableOid int, batchNum int) { pipe := fmt.Sprintf("%s_%d_%d", *pipeFile, tableOid, batchNum) logInfo(fmt.Sprintf("Oid %d, Batch %d: Closing pipe %s", tableOid, batchNum, pipe)) @@ -157,10 +161,14 @@ type oidWithBatch struct { type IRestoreHelper interface { getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) flushAndCloseRestoreWriter(pipeName string, oid int) error + + createPipe(pipe string) error closeAndDeletePipe(tableOid int, batchNum int) + getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) getRestorePipeWriter(currentPipe string) (*bufio.Writer, *os.File, error) checkForSkipFile(pipeFile string, tableOid int) bool + preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCount int) } type RestoreHelper struct{} @@ -169,11 +177,18 @@ func (RestoreHelper) checkForSkipFile(pipeFile string, tableOid int) bool { return utils.FileExists(fmt.Sprintf("%s_skip_%d", pipeFile, tableOid)) } +func (h *RestoreHelper) preloadCreatedPipesForRestore(oidWithBatchList []oidWithBatch, queuedPipeCount int) { + for i := 0; i < queuedPipeCount; i++ { + pipeName := fmt.Sprintf("%s_%d_%d", *pipeFile, oidWithBatchList[i].oid, oidWithBatchList[i].batch) + pipesMap[pipeName] = true + } +} + func doRestoreAgent() error { - return doRestoreAgentInternal(new(Helper), new(RestoreHelper)) + return doRestoreAgentInternal(new(RestoreHelper)) } -func doRestoreAgentInternal(helper IHelper, restoreHelper IRestoreHelper) error { +func doRestoreAgentInternal(restoreHelper IRestoreHelper) error { // We need to track various values separately per content for resize restore var segmentTOC map[int]*toc.SegmentTOC var tocEntries map[int]map[uint]toc.SegmentDataEntry @@ -259,7 +274,7 @@ func doRestoreAgentInternal(helper IHelper, restoreHelper IRestoreHelper) error } } - helper.preloadCreatedPipesForRestore(oidWithBatchList, *copyQueue) + restoreHelper.preloadCreatedPipesForRestore(oidWithBatchList, *copyQueue) var currentPipe string @@ -285,7 +300,7 @@ func doRestoreAgentInternal(helper IHelper, restoreHelper IRestoreHelper) error nextBatchNum := nextOidWithBatch.batch nextPipeToCreate := fmt.Sprintf("%s_%d_%d", *pipeFile, nextOid, nextBatchNum) logVerbose(fmt.Sprintf("Oid %d, Batch %d: Creating pipe %s\n", nextOid, nextBatchNum, nextPipeToCreate)) - err := helper.createPipe(nextPipeToCreate) + err := restoreHelper.createPipe(nextPipeToCreate) if err != nil { logError(fmt.Sprintf("Oid %d, Batch %d: Failed to create pipe %s\n", nextOid, nextBatchNum, nextPipeToCreate)) // In the case this error is hit it means we have lost the From 53402c7d3b8c1ed6f5dda629e4ea9e9b65daff2a Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Mon, 28 Oct 2024 09:26:54 +0500 Subject: [PATCH 27/29] Changes inspired by code review --- helper/helper_test.go | 13 ++++--------- helper/restore_helper.go | 10 +++++----- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/helper/helper_test.go b/helper/helper_test.go index d85ffba6f..8e28b843e 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -169,7 +169,7 @@ func (tp *testPluginCmd) hasProcess() bool { return tp.hasProcess_ } -func (pt *testPluginCmd) Pid() int { +func (pt *testPluginCmd) pid() int { return 42 } @@ -298,13 +298,13 @@ var _ = Describe("helper tests", func() { oidBatch := []oidWithBatch{{oid: 1, batch: 1}} steps := []helperTestStep{ - {restorePipeWriterArgExpect: "mock_1_1", restorePipeWriterResult: true, skipFileArgTableOid: 1, skipFileResult: false}, + {"mock_1_1", true, 1, false, "Can open single data file"}, } mockHelper := newHelperTest(oidBatch, steps) // Prepare and write the toc file - testDir := "" // Use local directory for the TOC file instead of default onr + testDir := "" // Use local directory for the TOC file instead of default *tocFile = fmt.Sprintf("%stest_toc.yaml", testDir) writeTestTOC(*tocFile) defer func() { @@ -321,12 +321,7 @@ var _ = Describe("helper tests", func() { // Call the function under test oidBatch := []oidWithBatch{{oid: 1, batch: 1}} steps := []helperTestStep{ - { - restorePipeWriterArgExpect: "mock_1_1", - restorePipeWriterResult: true, - skipFileArgTableOid: 1, - skipFileResult: false, - }, + {"mock_1_1", true, 1, false, "restores using multiple data files"}, } mockHelper := newHelperTest(oidBatch, steps) diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 6bca742d1..6cd19b48a 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -41,7 +41,7 @@ var ( ) /* IRestoreReader interface to wrap the underlying reader. - * readerType identifies how the reader can be used + * getReaderType() identifies how the reader can be used * SEEKABLE uses seekReader. Used when restoring from uncompressed data with filters from local filesystem * NONSEEKABLE and SUBSET types uses bufReader. * SUBSET type applies when restoring using plugin(if compatible) from uncompressed data with filters @@ -564,7 +564,7 @@ func (RestoreHelper) getRestoreDataReader(fileToRead string, objToc *toc.Segment return nil, err } if pluginCmd != nil { - logVerbose(fmt.Sprintf("Started plugin process (%d)", pluginCmd.Pid())) + logVerbose(fmt.Sprintf("Started plugin process (%d)", pluginCmd.pid())) } // Set the underlying stream reader in restoreReader @@ -626,11 +626,11 @@ func getSubsetFlag(fileToRead string, pluginConfig *utils.PluginConfig) bool { return true } -// pluginCmd is needed to keep track of readable stderr and whether the command +// IPluginCmd is needed to keep track of readable stderr and whether the command // has already been ended. type IPluginCmd interface { hasProcess() bool - Pid() int + pid() int Wait() error errLog() } @@ -644,7 +644,7 @@ func (p PluginCmd) hasProcess() bool { return p.Process != nil } -func (p PluginCmd) Pid() int { +func (p PluginCmd) pid() int { return p.Process.Pid } From cae0e7600deb1bf909511b07ed30c64b9856a16b Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Mon, 28 Oct 2024 09:33:44 +0500 Subject: [PATCH 28/29] Fix build --- helper/restore_helper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helper/restore_helper.go b/helper/restore_helper.go index 6cd19b48a..d1cc2f331 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -69,7 +69,7 @@ type RestoreReader struct { func (r *RestoreReader) waitForPlugin() error { var err error if r.pluginCmd != nil && r.pluginCmd.hasProcess() { - logVerbose(fmt.Sprintf("Waiting for the plugin process (%d)", r.pluginCmd.Pid())) + logVerbose(fmt.Sprintf("Waiting for the plugin process (%d)", r.pluginCmd.pid())) err = r.pluginCmd.Wait() if err != nil { logError(fmt.Sprintf("Plugin process exited with an error: %s", err)) From ac15111a8e48a6fee33208464ad5789fad074b7b Mon Sep 17 00:00:00 2001 From: Denis Kovalev Date: Thu, 14 Nov 2024 19:19:40 +0500 Subject: [PATCH 29/29] move closing pipe to increase locality --- helper/helper.go | 8 ++------ helper/helper_test.go | 4 ++++ helper/restore_helper.go | 12 +++++++++--- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/helper/helper.go b/helper/helper.go index af2d3ca04..b4bc1b937 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -240,14 +240,10 @@ func DoCleanup() { logVerbose("Encountered error closing error file: %v", err) } } - err := FlushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) - if err != nil { - logVerbose("Encountered error during cleanup: %v", err) - } for pipeName, _ := range pipesMap { logVerbose("Removing pipe %s", pipeName) - err = deletePipe(pipeName) + err := deletePipe(pipeName) if err != nil { logVerbose("Encountered error removing pipe %s: %v", pipeName, err) } @@ -255,7 +251,7 @@ func DoCleanup() { skipFiles, _ := filepath.Glob(fmt.Sprintf("%s_skip_*", *pipeFile)) for _, skipFile := range skipFiles { - err = utils.RemoveFileIfExists(skipFile) + err := utils.RemoveFileIfExists(skipFile) if err != nil { logVerbose("Encountered error during cleanup skip files: %v", err) } diff --git a/helper/helper_test.go b/helper/helper_test.go index 8e28b843e..d42b42e6e 100644 --- a/helper/helper_test.go +++ b/helper/helper_test.go @@ -140,6 +140,10 @@ func (h *restoreMockHelperImpl) flushAndCloseRestoreWriter(pipeName string, oid return nil } +func (*restoreMockHelperImpl) doRestoreAgentCleanup() { + // This was intentionaly left blank to support the IRestoreHelper interface +} + func (h *restoreMockHelperImpl) getRestoreDataReader(fileToRead string, objToc *toc.SegmentTOC, oidList []int) (IRestoreReader, error) { Expect(h.restoreData).ToNot(BeNil()) return h.restoreData, nil diff --git a/helper/restore_helper.go b/helper/restore_helper.go index d1cc2f331..4d1e30c22 100644 --- a/helper/restore_helper.go +++ b/helper/restore_helper.go @@ -161,6 +161,7 @@ type oidWithBatch struct { type IRestoreHelper interface { getOidWithBatchListFromFile(oidFileName string) ([]oidWithBatch, error) flushAndCloseRestoreWriter(pipeName string, oid int) error + doRestoreAgentCleanup() createPipe(pipe string) error closeAndDeletePipe(tableOid int, batchNum int) @@ -206,6 +207,8 @@ func doRestoreAgentInternal(restoreHelper IRestoreHelper) error { return err } + defer restoreHelper.doRestoreAgentCleanup() + // During a larger-to-smaller restore, we need to do multiple passes for each oid, so the table // restore goes into another nested for loop below. In the normal or smaller-to-larger cases, // this is equivalent to doing a single loop per table. @@ -463,11 +466,14 @@ func doRestoreAgentInternal(restoreHelper IRestoreHelper) error { return lastError } -func (RestoreHelper) flushAndCloseRestoreWriter(pipeName string, oid int) error { - return FlushAndCloseRestoreWriter(pipeName, oid) +func (rh *RestoreHelper) doRestoreAgentCleanup() { + err := rh.flushAndCloseRestoreWriter("Current writer pipe on cleanup", 0) + if err != nil { + logVerbose("Encountered error during cleanup: %v", err) + } } -func FlushAndCloseRestoreWriter(pipeName string, oid int) error { +func (RestoreHelper) flushAndCloseRestoreWriter(pipeName string, oid int) error { if writer != nil { writer.Write([]byte{}) // simulate writer connected in case of error err := writer.Flush()