You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/12/02 03:55:40 UTC

[GitHub] [apisix-dashboard] gxthrj opened a new pull request #947: fix: snowflake remove network dependency

gxthrj opened a new pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947


   Please answer these questions before submitting a pull request
   
   - Why submit this pull request?
   - [x] Bugfix
   - [ ] New feature provided
   - [ ] Improve performance
   
   - Related issues
   fix #938 
   ___
   ### Bugfix
   - Description
   remove network dependency for snowflake 
   
   - How to fix?
   Get IP in local
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] gxthrj commented on pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
gxthrj commented on pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#issuecomment-737040988


   > @gxthrj please take a look at the output of CI
   > 
   > ![image](https://user-images.githubusercontent.com/6814606/100832085-1d8c1c80-34a2-11eb-8c4f-d9971c07bdde.png)
   
   Fixed.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#discussion_r533889170



##########
File path: api/internal/utils/utils.go
##########
@@ -37,10 +38,13 @@ func init() {
 		}
 		salt = uint16(i)
 	}
-
+	ips, err := getLocalIPs()
+	if err != nil {
+		panic(err)
+	}
 	_sf = sonyflake.NewSonyflake(sonyflake.Settings{
 		MachineID: func() (u uint16, e error) {
-			return sumIP(GetOutboundIP()) + salt, nil
+			return sumIP(randomIP(ips)) + salt, nil

Review comment:
       👌




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] ShiningRush commented on a change in pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
ShiningRush commented on a change in pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#discussion_r533889484



##########
File path: api/internal/utils/utils.go
##########
@@ -37,10 +38,13 @@ func init() {
 		}
 		salt = uint16(i)
 	}
-
+	ips, err := getLocalIPs()
+	if err != nil {
+		panic(err)
+	}
 	_sf = sonyflake.NewSonyflake(sonyflake.Settings{
 		MachineID: func() (u uint16, e error) {
-			return sumIP(GetOutboundIP()) + salt, nil
+			return sumIP(randomIP(ips)) + salt, nil

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#discussion_r533885746



##########
File path: api/internal/utils/utils.go
##########
@@ -37,10 +38,13 @@ func init() {
 		}
 		salt = uint16(i)
 	}
-
+	ips, err := getLocalIPs()
+	if err != nil {
+		panic(err)
+	}
 	_sf = sonyflake.NewSonyflake(sonyflake.Settings{
 		MachineID: func() (u uint16, e error) {
-			return sumIP(GetOutboundIP()) + salt, nil
+			return sumIP(randomIP(ips)) + salt, nil

Review comment:
       may it meet 127.0.0.1 ？ 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] membphis commented on pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#issuecomment-736975705


   @ShiningRush please take a look at this RP when you have time ^_^
   
   we need to fix this issue today


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] ShiningRush commented on a change in pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
ShiningRush commented on a change in pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#discussion_r533945810



##########
File path: api/internal/utils/utils.go
##########
@@ -37,35 +38,50 @@ func init() {
 		}
 		salt = uint16(i)
 	}
-
+	ips, err := getLocalIPs()
+	if err != nil {
+		panic(err)
+	}
 	_sf = sonyflake.NewSonyflake(sonyflake.Settings{
 		MachineID: func() (u uint16, e error) {
-			return sumIP(GetOutboundIP()) + salt, nil
+			return sumIPs(ips) + salt, nil
 		},
 	})
 	if _sf == nil {
 		panic("sonyflake init failed")
 	}
 }
 
-func sumIP(ip net.IP) uint16 {
+func sumIPs(ips []net.IP) uint16 {
 	total := 0
-	for i := range ip {
-		total += int(ip[i])
+	for _, ip := range ips {
+		for i := range ip {
+			total += int(ip[i])
+		}
 	}
 	return uint16(total)
 }
 
-// Get preferred outbound ip of this machine
-func GetOutboundIP() net.IP {
-	conn, err := net.Dial("udp", "8.8.8.8:80")
+func getLocalIPs() ([]net.IP, error) {
+	var ips []net.IP
+	addrs, err := net.InterfaceAddrs()
 	if err != nil {
-		panic(err)
+		return ips, err
+	}
+	for _, a := range addrs {
+		if ipNet, ok := a.(*net.IPNet); ok && !ipNet.IP.IsLoopback() && ipNet.IP.To4() != nil {
+			ips = append(ips, ipNet.IP)
+		}
 	}
-	defer conn.Close()
+	return ips, nil
+}
 
-	localAddr := conn.LocalAddr().(*net.UDPAddr)
-	return localAddr.IP
+func randomIP(xs []net.IP) net.IP {

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] gxthrj commented on a change in pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#discussion_r533886741



##########
File path: api/internal/utils/utils.go
##########
@@ -37,10 +38,13 @@ func init() {
 		}
 		salt = uint16(i)
 	}
-
+	ips, err := getLocalIPs()
+	if err != nil {
+		panic(err)
+	}
 	_sf = sonyflake.NewSonyflake(sonyflake.Settings{
 		MachineID: func() (u uint16, e error) {
-			return sumIP(GetOutboundIP()) + salt, nil
+			return sumIP(randomIP(ips)) + salt, nil

Review comment:
       No, `127.0.0.1` is filtered out




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] membphis commented on pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#issuecomment-736999759


   @gxthrj please take a look at the output of CI
   
   ![image](https://user-images.githubusercontent.com/6814606/100832085-1d8c1c80-34a2-11eb-8c4f-d9971c07bdde.png)
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] membphis merged pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
membphis merged pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] gxthrj commented on a change in pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#discussion_r533940352



##########
File path: api/internal/utils/utils.go
##########
@@ -37,10 +38,13 @@ func init() {
 		}
 		salt = uint16(i)
 	}
-
+	ips, err := getLocalIPs()
+	if err != nil {
+		panic(err)
+	}
 	_sf = sonyflake.NewSonyflake(sonyflake.Settings{
 		MachineID: func() (u uint16, e error) {
-			return sumIP(GetOutboundIP()) + salt, nil
+			return sumIP(randomIP(ips)) + salt, nil

Review comment:
       OK, you convinced me.
   Done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] gxthrj commented on a change in pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#discussion_r533883940



##########
File path: api/internal/utils/utils.go
##########
@@ -37,10 +38,13 @@ func init() {
 		}
 		salt = uint16(i)
 	}
-
+	ips, err := getLocalIPs()
+	if err != nil {
+		panic(err)
+	}
 	_sf = sonyflake.NewSonyflake(sonyflake.Settings{
 		MachineID: func() (u uint16, e error) {
-			return sumIP(GetOutboundIP()) + salt, nil
+			return sumIP(randomIP(ips)) + salt, nil

Review comment:
       I think one is enough, it is for machineID, one or all IPs are the same :)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] codecov-io commented on pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#issuecomment-736984311


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/947?src=pr&el=h1) Report
   > Merging [#947](https://codecov.io/gh/apache/apisix-dashboard/pull/947?src=pr&el=desc) (7b9ff24) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/95f26122fee30d2fcbd18d98433b106c09e9bdca?el=desc) (95f2612) will **increase** coverage by `0.08%`.
   > The diff coverage is `68.75%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/947/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/947?src=pr&el=tree)
   
   ```diff
   @@            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     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/947?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/947/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | `52.50% <68.75%> (+0.88%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/947?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/947?src=pr&el=footer). Last update [95f2612...7b9ff24](https://codecov.io/gh/apache/apisix-dashboard/pull/947?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #947: fix: snowflake remove network dependency

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #947:
URL: https://github.com/apache/apisix-dashboard/pull/947#discussion_r533881123



##########
File path: api/internal/utils/utils.go
##########
@@ -37,10 +38,13 @@ func init() {
 		}
 		salt = uint16(i)
 	}
-
+	ips, err := getLocalIPs()
+	if err != nil {
+		panic(err)
+	}
 	_sf = sonyflake.NewSonyflake(sonyflake.Settings{
 		MachineID: func() (u uint16, e error) {
-			return sumIP(GetOutboundIP()) + salt, nil
+			return sumIP(randomIP(ips)) + salt, nil

Review comment:
       how about sum all local ips?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org