[patch] Test/internal/tcp #501

kevindiu · 2020-06-22T09:52:12Z

Author review required!!!!
Maybe performance test required before merging.

Description:

Sorry for this PR become so huge :(

This PR includes the following changes:

enhancement: fetch DNS cache in round robin order instead of random order (which may cause performance problem since it is not balanced)
enhancement: do not lookup DNS when the address passed to cachedDialer() is IP address
bug fix: DNS cache expire hook not executed
bug fix: dialerKeepAlive did not set properly
internal/tcp package test
code refactoring

The test coverage is over 90% in internal/net/tcp package.

When I working on this PR, I found a bug about sharing cache. and I created a PR for this bug fix. #560

Performance result:

Gateway replicas: 10
Agent replicas: 30
Dataset: random-786-100000
Concurrency: 32
Batch size: 100

Insert mode (100000 random vectors)

Nightly branch (20200713)
Average VPS for each run:

5395.421578
6424.425950
6425.912179
6461.443722
5988.220358
6493.199904
6486.264278
6335.389072
6129.457926
6659.673990

Variance

Total Average: 6279.9408957

PR-501
Average VPS for each run:

6217.897276
6673.575530
6976.387795
6890.498418
6487.006260
6585.224161
6734.012306
6966.123523
5937.095263
6751.386072

Variance

Total Average: 6621.9206604 (+ 5.4455 % )

Search Mode

Nightly branch (20200714)
Average VPS for each run:

55342.011671
55041.653094
54863.910108
55507.475455
54847.253156
55087.350757
54773.136435
55560.020191
55438.817729
55180.390630

Variance

Total Average: 55164.2019226

PR501
Average VPS for each run:

53006.025749
54954.973641
55533.508719
55152.016128
55107.615004
55149.334190
55255.700455
55688.780070
54430.523454
54487.434100

Variance

Total Average: 54876.591151 (- 0.521372124%)

Search Mode (with 20000 random data & 3 index replica)

Nightly branch (20200715):
Average VPS for each run:

54754.772502
54210.456157
55277.095015
54375.751102
54606.793579
54980.820024
55043.223001
54279.938913
54407.296268
54939.336378

Variance

Total Average: 54687.5482939

PR 501:
Average VPS for each run:

54635.047079
54744.703738
55256.844790
54916.153493
55260.394869
54986.154799
55572.723950
54315.390605
55549.817220
53304.934344

Variance

Total Average: 54854.2164888 (+ 3%)

Related Issue:

How Has This Been Tested?:

Environment:

Go Version: 1.14.3
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.11.5

Types of changes:

Bug fix [type/bug]
New feature [type/feature]
Add tests [type/test]
Security related changes [type/security]
Add documents [type/documentation]
Refactoring [type/refactoring]
Update dependencies [type/dependency]
Update benchmarks and performances [type/bench]
Update CI [type/ci]

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

Checklist:

I have read the CONTRIBUTING document.
I have checked open Pull Requests for the similar feature or fixes?
I have added tests and benchmarks to cover my changes.
I have ensured all new and existing tests passed.
I have commented my code, particularly in hard-to-understand areas
I have updated the documentation accordingly.

pull-assistant · 2020-06-22T09:52:15Z

Best reviewed: commit by commit

Optimal code review plan (9 warnings)

refactor

internal/net/tcp/dialer.go 75% changes removed in fix

internal/net/tcp/dialer.go 60% changes removed in fix

fix template and rename file

add option test and missing file

refactor

internal/net/tcp/dialer.go 46% changes removed in fix

fix

internal/net/tcp/dialer.go 50% changes removed in add test case

use thread safe

fix

Apply suggestions from code review

...nternal/net/tcp/dialer_test.go 80% changes removed in fix

fix

...nternal/net/tcp/dialer_test.go 56% changes removed in add connection check...

fix

internal/net/tcp/dialer.go 60% changes removed in Update internal/net/...

fix

internal/net/tcp/dialer.go 67% changes removed in fix

fix

revert change of cache bug fix

Merge remote-tracking branch 'origin/master' into test/internal/tcp

fix lint

internal/net/tcp/dialer.go 50% changes removed in fix

fix

🤖 Update license headers / Format go codes and yaml files

fix comment

Apply suggestions from code review

add connection check testing

fix

Merge branch 'master' into test/internal/tcp

fix

Merge branch 'master' into test/internal/tcp

add comment to option.go

Update internal/net/tcp/option.go

Merge branch 'master' into test/internal/tcp

fix deepsource

fix

Apply suggestions from code review

remove failed and unused test

Update internal/net/tcp/dialer.go

rename function name

fix comment

Powered by Pull Assistant. Last update 1572b71 ... c5b390d. Read the comment docs.

github-actions · 2020-06-22T09:58:05Z

internal/net/tcp/dialer_test.go

 	"github.com/vdaas/vald/internal/errors"
 	"github.com/vdaas/vald/internal/net"

 	"go.uber.org/goleak"
 )

+var (
+	goleakIgnoreOptions = []goleak.Option{


[golangci] _{reported by reviewdog 🐶}
goleakIgnoreOptions is a global variable (gochecknoglobals)

internal/net/tcp/dialer.go

internal/net/tcp/dialer_test.go

github-actions · 2020-06-22T09:58:06Z

internal/net/tcp/dialer_test.go

+				return !(x == nil && y != nil) || !(y == nil && x != nil)
+			}),
+			cmp.Comparer(func(x, y tls.Config) bool {
+				return reflect.DeepEqual(x, y)


[golangci] _{reported by reviewdog 🐶}
copylocks: call of reflect.DeepEqual copies lock value: crypto/tls.Config contains sync.Once contains sync.Mutex (govet)

codecov · 2020-06-22T10:01:20Z

Codecov Report

Merging #501 into master will increase coverage by 0.60%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master     #501      +/-   ##
==========================================
+ Coverage   11.08%   11.69%   +0.60%     
==========================================
  Files         403      404       +1     
  Lines       20930    20928       -2     
==========================================
+ Hits         2321     2448     +127     
+ Misses      18340    18213     -127     
+ Partials      269      267       -2

Impacted Files	Coverage Δ
internal/errors/net.go	`0.00% <0.00%> (ø)`
internal/net/tcp/dialer.go	`94.93% <96.22%> (+94.93%)`	⬆️
internal/net/tcp/option.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba21911...c5b390d. Read the comment docs.

kevindiu · 2020-06-23T07:20:15Z

/rebase

vdaas-ci · 2020-06-23T07:20:53Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/tcp

internal/net/tcp/dialer.go

kevindiu · 2020-06-25T01:07:57Z

/rebase

vdaas-ci · 2020-06-25T01:08:32Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/tcp

internal/net/tcp/dialer.go

kevindiu · 2020-06-26T09:50:23Z

/rebase

vdaas-ci · 2020-06-26T09:51:23Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/tcp

kevindiu · 2020-06-30T09:21:44Z

/rebase

vdaas-ci · 2020-06-30T09:22:21Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/tcp

internal/net/tcp/dialer.go

kevindiu · 2020-07-14T07:53:51Z

@vankichi

Is this result statistically significant?

I think no. It decrease 0.5% searching performance but increase 5% insert performance

And, could you give us the variance value not only ave.?

I have updated the description for more details.Please tell me if you have more questions :)

rinx · 2020-07-14T08:33:30Z

Insert: nightly
Insert: pr-501
Search: nightly
Search: pr-501

kevindiu · 2020-07-14T08:44:48Z

@rinx thank you!

kevindiu · 2020-07-17T02:39:49Z

/rebase

vdaas-ci · 2020-07-17T02:41:01Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/tcp

vdaas-ci · 2020-07-17T02:41:06Z

[REBASE] Failed to rebase.

internal/errors/net.go

github-actions · 2020-07-17T02:51:54Z

internal/net/tcp/dialer_test.go

+		}(),
+		func() test {
+			tc := &tls.Config{
+				InsecureSkipVerify: true,


[golangci] _{reported by reviewdog 🐶}
G402: TLS InsecureSkipVerify set true. (gosec)

internal/errors/net.go

kpango

LGTM

kpango · 2020-07-17T05:25:19Z

internal/net/tcp/dialer.go

+
+func (d *dialer) cacheExpireHook(ctx context.Context, addr string) {
+	if err := safety.RecoverFunc(func() error {
+		_, err1 := d.lookup(ctx, addr)


please do not use numbering variable

Suggested change

_, err1 := d.lookup(ctx, addr)

_, err1 := d.lookup(ctx, addr)

need fix

internal/net/tcp/dialer.go

kpango · 2020-07-17T05:37:05Z

internal/net/tcp/dialer.go

 	if err := safety.RecoverFunc(func() error {
-		_, err1 := d.lookup(ctx, addr)
-		return err1
+		_, lerr := d.lookup(ctx, addr)
+		return lerr


please use named return lerr looks dirty

internal/net/tcp/dialer.go

kpango · 2020-07-17T05:44:15Z

internal/net/tcp/dialer.go

+
+// GetIP returns the next cached IP address in round robin order.
+// It starts getting the index 1 cache instead of index 0 cache.
+func (d *dialerCache) GetIP() string {


could you please change function naming to IP()

kpango · 2020-07-17T05:47:44Z

internal/net/tcp/dialer.go

+	if d.dnsCache && !isIP {
+		if dc, err := d.lookup(dctx, host); err == nil {
+			for i := uint32(0); i < dc.Len(); i++ {
+				if conn, err := d.dial(dctx, network, fmt.Sprintf("%s:%d", dc.IP(), port)); err == nil {


fmt.Sprintf is slow is there any plan to improve performance?

I think strconv.FormatInt is better than Sprint

fixed! thank you :)

kpango

LGTM

github-actions bot added area/internal team/set SET team size/XL type/test Adding tests priority/low labels Jun 22, 2020

github-actions bot reviewed Jun 22, 2020

View reviewed changes

vdaas-ci force-pushed the test/internal/tcp branch from 452eb3e to 805e0de Compare June 23, 2020 07:21

github-actions bot reviewed Jun 23, 2020

View reviewed changes

internal/net/tcp/dialer.go Show resolved Hide resolved

github-actions bot added size/XXL and removed size/XL labels Jun 24, 2020

vdaas-ci force-pushed the test/internal/tcp branch from b67df85 to ebe6016 Compare June 25, 2020 01:08

github-actions bot reviewed Jun 25, 2020

View reviewed changes

internal/net/tcp/dialer.go Outdated Show resolved Hide resolved

kpango reviewed Jun 25, 2020

View reviewed changes

internal/net/tcp/dialer.go Outdated Show resolved Hide resolved

internal/net/tcp/dialer.go Outdated Show resolved Hide resolved

kpango reviewed Jun 25, 2020

View reviewed changes

internal/net/tcp/dialer.go Outdated Show resolved Hide resolved

kevindiu commented Jun 26, 2020

View reviewed changes

internal/net/tcp/dialer.go Show resolved Hide resolved

kpango force-pushed the test/internal/tcp branch from 2c04349 to d73185f Compare June 26, 2020 02:54

vdaas-ci force-pushed the test/internal/tcp branch from 02b1741 to 65ea3eb Compare June 26, 2020 09:51

vdaas-ci force-pushed the test/internal/tcp branch from 65ea3eb to 6918cb5 Compare June 30, 2020 09:22

kpango reviewed Jul 1, 2020

View reviewed changes

internal/net/tcp/dialer.go Show resolved Hide resolved

kevindiu commented Jul 1, 2020

View reviewed changes

internal/net/tcp/dialer.go Outdated Show resolved Hide resolved

Yusuke Kato and others added 2 commits July 16, 2020 18:25

Merge branch 'master' into test/internal/tcp

0bac1e7

fix deepsource

82ae5e3

kevindiu dismissed vankichi’s stale review via 82ae5e3 July 17, 2020 02:37

github-actions bot reviewed Jul 17, 2020

View reviewed changes

internal/errors/net.go Show resolved Hide resolved

fix

79b9b36

github-actions bot reviewed Jul 17, 2020

View reviewed changes

kevindiu commented Jul 17, 2020

View reviewed changes

internal/errors/net.go Show resolved Hide resolved

kevindiu added 2 commits July 17, 2020 12:02

Apply suggestions from code review

0b443b6

remove failed and unused test

b353a7c

kpango previously approved these changes Jul 17, 2020

View reviewed changes

kpango reviewed Jul 17, 2020

View reviewed changes

kevindiu commented Jul 17, 2020

View reviewed changes

internal/net/tcp/dialer.go Outdated Show resolved Hide resolved

Update internal/net/tcp/dialer.go

393c7f7

kpango reviewed Jul 17, 2020

View reviewed changes

kevindiu commented Jul 17, 2020

View reviewed changes

internal/net/tcp/dialer.go Outdated Show resolved Hide resolved

Update internal/net/tcp/dialer.go

550685e

kpango reviewed Jul 17, 2020

View reviewed changes

rename function name

d0a06d8

kpango reviewed Jul 17, 2020

View reviewed changes

fix comment

c5b390d

kpango approved these changes Jul 17, 2020

View reviewed changes

kpango merged commit 08c5fdf into master Jul 17, 2020

kpango deleted the test/internal/tcp branch July 17, 2020 06:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[patch] Test/internal/tcp #501

[patch] Test/internal/tcp #501

kevindiu commented Jun 22, 2020 •

edited

Loading

pull-assistant bot commented Jun 22, 2020 •

edited

Loading

github-actions bot Jun 22, 2020

github-actions bot Jun 22, 2020

codecov bot commented Jun 22, 2020 •

edited

Loading

kevindiu commented Jun 23, 2020

vdaas-ci commented Jun 23, 2020

kevindiu commented Jun 25, 2020

vdaas-ci commented Jun 25, 2020

kevindiu commented Jun 26, 2020

vdaas-ci commented Jun 26, 2020

kevindiu commented Jun 30, 2020

vdaas-ci commented Jun 30, 2020

kevindiu commented Jul 14, 2020 •

edited

Loading

rinx commented Jul 14, 2020

kevindiu commented Jul 14, 2020

kevindiu commented Jul 17, 2020

vdaas-ci commented Jul 17, 2020

vdaas-ci commented Jul 17, 2020

github-actions bot Jul 17, 2020

kpango left a comment

kpango Jul 17, 2020

kevindiu Jul 17, 2020

kpango Jul 17, 2020

kpango Jul 17, 2020

kevindiu Jul 17, 2020

kpango Jul 17, 2020

kpango Jul 17, 2020

kevindiu Jul 17, 2020

kpango left a comment

	_, err1 := d.lookup(ctx, addr)
	_, err1 := d.lookup(ctx, addr)

[patch] Test/internal/tcp #501

[patch] Test/internal/tcp #501

Conversation

kevindiu commented Jun 22, 2020 • edited Loading

Description:

Insert mode (100000 random vectors)

Search Mode

Search Mode (with 20000 random data & 3 index replica)

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

pull-assistant bot commented Jun 22, 2020 • edited Loading

github-actions bot Jun 22, 2020

Choose a reason for hiding this comment

github-actions bot Jun 22, 2020

Choose a reason for hiding this comment

codecov bot commented Jun 22, 2020 • edited Loading

Codecov Report

kevindiu commented Jun 23, 2020

vdaas-ci commented Jun 23, 2020

kevindiu commented Jun 25, 2020

vdaas-ci commented Jun 25, 2020

kevindiu commented Jun 26, 2020

vdaas-ci commented Jun 26, 2020

kevindiu commented Jun 30, 2020

vdaas-ci commented Jun 30, 2020

kevindiu commented Jul 14, 2020 • edited Loading

rinx commented Jul 14, 2020

kevindiu commented Jul 14, 2020

kevindiu commented Jul 17, 2020

vdaas-ci commented Jul 17, 2020

vdaas-ci commented Jul 17, 2020

github-actions bot Jul 17, 2020

Choose a reason for hiding this comment

kpango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpango left a comment

Choose a reason for hiding this comment

kevindiu commented Jun 22, 2020 •

edited

Loading

pull-assistant bot commented Jun 22, 2020 •

edited

Loading

codecov bot commented Jun 22, 2020 •

edited

Loading

kevindiu commented Jul 14, 2020 •

edited

Loading