Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor writeto db to smaller function #399

Merged
merged 6 commits into from
Oct 15, 2024

Conversation

yokofly
Copy link
Contributor

@yokofly yokofly commented Oct 9, 2024

I intend to split as 2 prs for #35
the first part is to refactor the writetodb function, I have run some tests, probably the CI will run a more compatible suite.

I conduct a more direct test to verify the temp table: insert a big table, stop with ctrl+c during the insert, and collect the log
this is the whole log difference(between release 1.2.20 and commit ac8802a), from the log seems all good
https://www.diffchecker.com/f4YCxD9W/

@yokofly yokofly changed the title Feature/refactor writeto db refactor writeto db to smaller function Oct 9, 2024
@flarco
Copy link
Collaborator

flarco commented Oct 9, 2024

Thanks, started to test locally but dealing with another issue.
I'm travelling this week so I may not get to it till this weekend or next week.

Also, there are a few instances of the use of g.Error that need fixing.

For example, instead of g.Error("Could not perform upsert from temp: %v", err) could you use g.Error(err, "Could not perform upsert from temp")? It's a wrapper that I use for error logging/stack tracing, if the first param is anerror type, it treats it differently.

@yokofly
Copy link
Contributor Author

yokofly commented Oct 9, 2024

hahaha, let's enjoy the trip first!
Will adjust the error wrapper.

@flarco flarco mentioned this pull request Oct 9, 2024
@yokofly
Copy link
Contributor Author

yokofly commented Oct 15, 2024

I locally performed some simple tests with clickhouse to clickhouse

@flarco
Copy link
Collaborator

flarco commented Oct 15, 2024

Tested, all good 👍 .

@flarco flarco merged commit 34e95c1 into slingdata-io:1.2.21 Oct 15, 2024
Comment on lines +258 to +264
if cnt != tCnt {
err = g.Error(err, "inserted in temp table but table count (%d) != stream count (%d). Records missing/mismatch. Aborting", tCnt, cnt)
return 0, err
} else if tCnt == 0 && len(sampleData.Rows) > 0 {
err = g.Error(err, "Loaded 0 records while sample data has %d records. Exiting.", len(sampleData.Rows))
return 0, err
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this code is buggy, sorry about that, I find the err will pass to the internal, and got something like

WRN NewError called with nil error:

old code

	tCnt, _ := tgtConn.GetCount(tableTmp.FullName())
	if cnt != tCnt {
		err = g.Error("inserted in temp table but table count (%d) != stream count (%d). Records missing/mismatch. Aborting", tCnt, cnt)
		return
	} else if tCnt == 0 && len(sampleData.Rows) > 0 {
		err = g.Error("Loaded 0 records while sample data has %d records. Exiting.", len(sampleData.Rows))
		return
	}

yokofly pushed a commit to yokofly/sling-cli that referenced this pull request Oct 18, 2024
…tetoDB

refactor writeto db to smaller function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants