[Draft] Error Policy for import data with stash-and-continue for ingestion errors #2639

makalaaneesh · 2025-05-20T13:31:25Z

Describe the changes in this pull request

https://docs.google.com/document/d/1KqQbIoQqQmlxIHql5Zd8aOyw8bP4j82T7Inw7tYMomg/edit?tab=t.0#heading=h.4p1zl0z1krwh

Overview of change:

Introduce error policies.
Introduce import data error handlers (one corresponding to each policy that we support)
ImportDataStashAndContinueHandler.HandleBatchIngestionError does the following
- mark batch as errored out (.E suffix)
- add error message to the batch file
All errored batches will remain under import-data-state folder. A new folder under data/errors/<table-name>/<file-name> will contain symlinks to the errored batch files. This is to give a single view only of the errored files. In future this folder will also contain the processing errors.

Overall Task list:

basic framework
- base interfaces
- errors during ingestion (when ingesting batch to YB)
- errors during processing (reading and transforming rows before batch creation )
- CLI and config flags
errors during ingestion
- basic abort policy
  - basic test for regression
  - tests
- stash and continue policy
  - mark batch as error
  - resumption
  - proper file structure with symlinks to batch files
  - add error message to batch file
  - move error message to the beginning of the batch file
  - tests
    - basic test on importBatch - error is not thrown.
    - multiple batches - error in single batch (first, middle, last), resumption with batch error,
    - multiple tasks - task completed with errors should not be re-picked
    - error message included in batch.
errors during processing
- basic abort policy
- stash and continue policy
- large rows?
- csv errors? (csvmaxreaderbuffer?)
status commands
- import data status
- get data-migration -report
end-migration
- back up error files as well
integrate with -on-primary-key-conflit
- change flag option from ERROR -> ERROR_POLICY
- handle partial errors

Describe if there are any user-facing changes

How was this pull request tested?

Does your PR have changes in callhome/yugabyted payloads? If so, is the payload version incremented?

Does your PR have changes that can cause upgrade issues?

Component	Breaking changes?
MetaDB	Yes/No
Name registry json	Yes/No
Data File Descriptor Json	Yes/No
Export Snapshot Status Json	Yes/No
Import Data State	Yes/No
Export Status Json	Yes/No
Data .sql files of tables	Yes/No
Export and import data queue	Yes/No
Schema Dump	Yes/No
AssessmentDB	Yes/No
Sizing DB	Yes/No
Migration Assessment Report Json	Yes/No
Callhome Json	Yes/No
YugabyteD Tables	Yes/No
TargetDB Metadata Tables	Yes/No

makalaaneesh · 2025-05-26T09:22:59Z

yb-voyager/cmd/importData.go

@@ -537,6 +540,12 @@ func importData(importFileTasks []*ImportFileTask) {
 		}
 	}

+	exportDirDataDir := filepath.Join(exportDir, "data")


todo: move this to a common func

In import data file, data dir is a configurable argument so its not inside export-dir

Correct, but that's not a concern, right? we always create a data/ folder. in import-data-file, it will be empty, and that's okay?

makalaaneesh · 2025-05-27T11:00:13Z

yb-voyager/src/errorpolicy/policy.go

+limitations under the License.
+*/
+
+package errorpolicy


probably an overkill of a package. Thinking about this in a follow-up PR.
Perhaps just put it inside constants?

Yes, it is not a heavy component that has a lot of functionality, as the actual functionality is in Import handlers, so it doesn't feel like it should be a separate package. I think some constants and functions in utils should suffice for this.

agreed, I was thinking of two options:

put it in constants package. (or create a new enums package)

put it in importdata package.

Going with importdata package as it's only going to be used in that package.

priyanshi-yb

Haven't reviewed it thoroughly, some comments on overall structure and flow.

priyanshi-yb · 2025-05-26T15:08:00Z

yb-voyager/src/importdata/errorHandlers.go

+	return &ImportDataAbortHandler{}
+}
+
+func (handler *ImportDataAbortHandler) ShouldAbort() bool {


Why will we have a check Abort in all handlers, that's the type of Handler right?

As discussed,

The alternative would be to type assert to the concrete type before we can check. ShouldAbort() feels slightly cleaner and it also makes sense to be part of the handler interface.

Theoretically, you could have more than one handlers wherein we might need to abort (for example, a SendSlackNotificationAndAbort handler ). Therefore, a func ShouldAbort() makes sense.

yb-voyager/cmd/importData.go

priyanshi-yb · 2025-05-26T18:30:50Z

yb-voyager/cmd/importDataFileTaskImporter.go

+		if err2 != nil {
+			utils.ErrExit("handling error for batch: %q into %s: %s", batch.FilePath, batch.TableNameTup, err2)
+		}
+		if fti.errorHandler.ShouldAbort() {


If we are having this function to check if it's a AbortHandler, should we check this first thing here and not do any handling etc..?

Although theoretically we can have handlers where we might need to handle and then abort, I'll simplify this flow for now.
Makes sense to handle only if we are not aborting. I will clean this up

priyanshi-yb · 2025-05-27T12:00:48Z

yb-voyager/src/errorpolicy/policy.go

+limitations under the License.
+*/
+
+package errorpolicy


Yes, it is not a heavy component that has a lot of functionality, as the actual functionality is in Import handlers, so it doesn't feel like it should be a separate package. I think some constants and functions in utils should suffice for this.

priyanshi-yb · 2025-05-27T12:02:35Z

yb-voyager/cmd/importDataState.go

 	for _, batch := range batches {
 		if batch.IsDone() {
 			doneCount++
 		} else if batch.IsInterrupted() {
 			interruptedCount++
+		} else if batch.IsErrored() {


What if someone fixes the batch and it gets ingested properly, will it then change the status, etc, for that scenario?

That is the workflow for dealing with stashed errors. That is out of scope for this PR (and overall task)

priyanshi-yb · 2025-05-28T05:38:54Z

yb-voyager/cmd/importData.go

@@ -537,6 +540,12 @@ func importData(importFileTasks []*ImportFileTask) {
 		}
 	}

+	exportDirDataDir := filepath.Join(exportDir, "data")


In import data file, data dir is a configurable argument so its not inside export-dir

priyanshi-yb · 2025-05-28T05:51:38Z

yb-voyager/src/importdata/errorHandlers.go

+}
+
+func (handler *ImportDataStashAndContinueHandler) getTaskErrorsFolderPath(batch ErroredBatch, importFileTaskId string) string {
+	return filepath.Join(handler.dataDir, "errors", batch.GetTableName().ForMinOutput(), importFileTaskId)


I think we shouldn't use the data dir for this as datadir is user's directory in case of import data file (local option) and is S3/GCS/etc.. buckets in cloud option.So, we should use import data state which is voyager's metadir.

I thought about this, but

the metainfo directory is an internal directory for voyager to maintain some state and metadata. i don't think it's a good idea to expose that to the user. We should be free to change that as we please. (i know we already do expose that, but only when there is an error).

It's cleaner if the directory that we point the user to for the errors, is dedicated and at the top-level (data/errors as opposed to under /metainfo/import_data_state/target_db_importer/ which seems pretty internal.

Do you see any concerns if we put it under data/errors ? As i mentioned in the other comment, in case of import-data-file, that directory will be empty, but i don't see that necessarily as a problem.

makalaaneesh added 13 commits May 19, 2025 15:07

base

a9eef63

flow via import data task importer

aed9d77

bug fix

c134e5d

error batch state

cbaf646

refactor packages

68dbf03

pick batches correctly when resuming

93a4b28

sym link

2e2c5ce

refactor error directory path

a8eae09

append error string

a99ef65

minor log fix

5b97b3e

Merge branch 'main' into aneesh/error-policy-base

89865a6

Default error policy

58c1297

fix tests

2921c0a

makalaaneesh commented 8000 May 26, 2025

View reviewed changes

makalaaneesh added 3 commits May 26, 2025 15:01

fix log

593b795

regression test

54c2750

basic regression test to assert that it errors out on error

ae07804

makalaaneesh commented May 27, 2025

View reviewed changes

priyanshi-yb reviewed May 27, 2025

View reviewed changes

review comment - restructure

dfa538a

priyanshi-yb reviewed May 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Draft] Error Policy for import data with stash-and-continue for ingestion errors #2639

[Draft] Error Policy for import data with stash-and-continue for ingestion errors #2639

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Draft] Error Policy for import data with stash-and-continue for ingestion errors #2639

Are you sure you want to change the base?

[Draft] Error Policy for import data with stash-and-continue for ingestion errors #2639

Conversation

Uh oh!

Describe the changes in this pull request

Describe if there are any user-facing changes

How was this pull request tested?

Does your PR have changes in callhome/yugabyted payloads? If so, is the payload version incremented?

Does your PR have changes that can cause upgrade issues?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!