-
Notifications
You must be signed in to change notification settings - Fork 532
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
fix: snowflake remove network dependency #947
Conversation
api/internal/utils/utils.go
Outdated
_sf = sonyflake.NewSonyflake(sonyflake.Settings{ | ||
MachineID: func() (u uint16, e error) { | ||
return sumIP(GetOutboundIP()) + salt, nil | ||
return sumIP(randomIP(ips)) + salt, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about sum all local ips?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one is enough, it is for machineID, one or all IPs are the same :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may it meet 127.0.0.1 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, 127.0.0.1
is filtered out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a risk that different instance may be get the same IP in multiple network environment (such as virtual network ), calculating all IP would be better when we can not confirm which is physical network interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, you convinced me.
Done.
@ShiningRush please take a look at this RP when you have time ^_^ we need to fix this issue today |
Codecov Report
@@ Coverage Diff @@
## master #947 +/- ##
==========================================
+ Coverage 43.10% 43.18% +0.08%
==========================================
Files 18 18
Lines 1290 1299 +9
==========================================
+ Hits 556 561 +5
- Misses 642 644 +2
- Partials 92 94 +2
Continue to review full report at Codecov.
|
@gxthrj please take a look at the output of CI |
Fixed. |
api/internal/utils/utils.go
Outdated
|
||
localAddr := conn.LocalAddr().(*net.UDPAddr) | ||
return localAddr.IP | ||
func randomIP(xs []net.IP) net.IP { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we can remove unused code and test case.
BTW: this is not a really random
, because here use the default random seed, so it will always output a certain int sequence.If we need to pick a ip, pick the first one would be better, it help to debug program.
Please answer these questions before submitting a pull request
Why submit this pull request?
Bugfix
New feature provided
Improve performance
Related issues
fix panic: dial udp 8.8.8.8:80: connect: network is unreachable #938
Bugfix
Description
remove network dependency for snowflake
How to fix?
Get IP in local