Skip to content

Conversation

@njbestway
Copy link
Contributor

This PR is to fix 2 problems:

  1. It will panic in the func WritePoint when All DBServers are Down. The resp maybe nil when any Err ocurrs.
  2. It should check whether the callback func is nil or not indeed. Just making func call using the callback directly doesn't seem friendly.It proves to be slightly confusing.

@njbestway njbestway force-pushed the fix-WritePoint-panic-of-nilpointer branch from b1ac426 to 341dd34 Compare June 28, 2024 13:08
if callback != nil {
callback(err)
}
return err
Copy link
Member

@hezhangjian hezhangjian Jun 28, 2024

Choose a reason for hiding this comment

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

Suggested change
return err
return nil

I prefer to return nil here since we decide to handle this error in callback.
And it's clear that anyone who didn't pass callback will miss error, callback should be passed in production.

@hezhangjian hezhangjian force-pushed the fix-WritePoint-panic-of-nilpointer branch from 341dd34 to 84ee03b Compare June 28, 2024 23:10
@hezhangjian hezhangjian force-pushed the fix-WritePoint-panic-of-nilpointer branch from 84ee03b to 8b4f350 Compare June 28, 2024 23:12
@hezhangjian
Copy link
Member

hezhangjian commented Jun 28, 2024

I changed it to return nil and merged this PR because the previous code decided to handle this error in the callback. I think it's quite clear that anyone who doesn't pass a callback will miss the error. The callback should be passed in production. Feel free to open an issue or continue in #79
to discuss it

@hezhangjian hezhangjian merged commit f39ed79 into openGemini:main Jun 28, 2024
@njbestway
Copy link
Contributor Author

I changed it to return nil and merged this PR because the previous code decided to handle this error in the callback. I think it's quite clear that anyone who doesn't pass a callback will miss the error. The callback should be passed in production. Feel free to open an issue or continue in #79 to discuss it

Agree! Nobody wants to dealwith both the err func Passed and the err Returned.

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