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.

Sign up for GitHub

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

Fix: nil pointer exception while sending the callhome payload for import data to source/source-replica #2371

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −6
Open

Fix: nil pointer exception while sending the callhome payload for import data to source/source-replica #2371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions yb-voyager/cmd/importDataToSource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,9 @@ func packAndSendImportDataToSourcePayload(status string, errorMsg string) {

sourceDBDetails := callhome.SourceDBDetails{
DBType: tconf.TargetDBType,
DBVersion: targetDBDetails.DBVersion,
}
if targetDBDetails != nil {
sourceDBDetails.DBVersion = targetDBDetails.DBVersion
Comment on lines 99 to +103
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe use a ternary operator for the check

sourceDBDetails := callhome.SourceDBDetails{
		DBType:    tconf.TargetDBType,
		DBVersion: lo.Ternary(targetDBDetails != nil,  targetDBDetails.DBVersion, ""),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't do this; it doesn't work if we do nil check in this function as it still tries to run the if clause. refer https://go.dev/play/p/sgeATTvMgMP

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting.
So this is because Go follows strict/eager evaluation of function argument

}
payload.SourceDBDetails = callhome.MarshalledJsonString(sourceDBDetails)

Expand All @@ -112,7 +114,7 @@ func packAndSendImportDataToSourcePayload(status string, errorMsg string) {

importDataPayload.Phase = importPhase

if importPhase != dbzm.MODE_SNAPSHOT {
if importPhase != dbzm.MODE_SNAPSHOT && statsReporter != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the case when statsReporter will be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be nil until it is initialized. If there is any failure or exit before that, this function will try to send the payload, and we can get a nil pointer exception in such a case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. I was think can we introduce some sort of error handling here.
Golang doesn't have try catch sort of thing, but they have combination of defer and recover to recover the program from panic state.

I think for this codepath it makes sense to handle panic, not think about other places if we can use this.

what do you guys think? @makalaaneesh @priyanshi-yb

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://www.honeybadger.io/blog/go-exception-handling/

importDataPayload.EventsImportRate = statsReporter.EventsImportRateLast3Min
importDataPayload.TotalImportedEvents = statsReporter.TotalEventsImported
}
Expand Down
10 changes: 6 additions & 4 deletions yb-voyager/cmd/importDataToSourceReplica.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,11 @@ func packAndSendImportDataToSrcReplicaPayload(status string, errorMsg string) {
payload.MigrationType = LIVE_MIGRATION

sourceDBDetails := callhome.SourceDBDetails{
DBType: tconf.TargetDBType,
DBVersion: targetDBDetails.DBVersion,
Role: "replica",
DBType: tconf.TargetDBType,
Role: "replica",
}
if targetDBDetails != nil {
sourceDBDetails.DBVersion = targetDBDetails.DBVersion
}
payload.SourceDBDetails = callhome.MarshalledJsonString(sourceDBDetails)

Expand All @@ -128,7 +130,7 @@ func packAndSendImportDataToSrcReplicaPayload(status string, errorMsg string) {

importDataPayload.Phase = importPhase

if importPhase != dbzm.MODE_SNAPSHOT {
if importPhase != dbzm.MODE_SNAPSHOT && statsReporter != nil {
importDataPayload.EventsImportRate = statsReporter.EventsImportRateLast3Min
importDataPayload.TotalImportedEvents = statsReporter.TotalEventsImported
}
Expand Down
Loading