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.

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

[patch] bugfix gateway & internal/net/grpc #569

Merged
merged 1 commit into from
Jul 10, 2020

Conversation

kpango
Copy link
Collaborator

@kpango kpango commented Jul 10, 2020

Signed-off-by: kpango i.can.feel.gravity@gmail.com

Description:

in this PR I fixed 4 bugs and added 1 feature
bugs

  1. grpc pool package sometimes panics due to divide by zero value
  2. when the user inserts empty vector agent will suddenly dies
  3. SearchByID operation in gateway has a logical problem
    • gateway broadcasts SearchByID operation to Agent, but if some agent doesn't have ID's vector this agent returns empty which decreases accuracy, now gateway fetchs Vector from backup manager then Broadcasts Search request using vector
    • The SearchByVectorID result may not accurate  #398
  4. discoverer/k8s sometime panics if there is no Pod in Node

feature

  1. some environment has multiple dns on their network I added dns resolver enable/disable configuration

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
Copy link

pull-assistant bot commented Jul 10, 2020

Score: 1.00

Best reviewed: commit by commit


Optimal code review plan

     [patch] bugfix gateway & internal/net/grpc

Powered by Pull Assistant. Last update 6931f99 ... 6931f99. Read the comment docs.

@vdaas-ci
Copy link
Collaborator

[WARNING:INTCFG] Changes in interal/config may require you to change Helm charts. Please check.

@vdaas-ci
Copy link
Collaborator

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 💌 /changelog - add changelog comment
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • /rebase - rebase master

@@ -80,6 +81,13 @@ func WithConnectionPoolRebalanceDuration(dur string) Option {
}
}


Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
File is not goimports-ed (goimports)

@@ -80,6 +81,13 @@ func WithConnectionPoolRebalanceDuration(dur string) Option {
}
}


Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
File is not gofmt-ed with -s (gofmt)

@codecov
Copy link

codecov bot commented Jul 10, 2020

Codecov Report

Merging #569 into master will increase coverage by 2.44%.
The diff coverage is 6.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
+ Coverage    7.84%   10.28%   +2.44%     
==========================================
  Files         388      403      +15     
  Lines       19945    20935     +990     
==========================================
+ Hits         1565     2154     +589     
- Misses      18156    18517     +361     
- Partials      224      264      +40     
Impacted Files Coverage Δ
internal/cache/cacher/cacher.go 100.00% <ø> (+100.00%) ⬆️
internal/cache/gache/gache.go 0.00% <0.00%> (ø)
internal/cache/gache/option.go 0.00% <0.00%> (-6.67%) ⬇️
internal/client/discoverer/discover.go 0.00% <0.00%> (ø)
internal/config/blob.go 0.00% <0.00%> (ø)
internal/config/grpc.go 0.00% <0.00%> (ø)
internal/config/log.go 100.00% <ø> (+100.00%) ⬆️
internal/config/mysql.go 100.00% <ø> (+100.00%) ⬆️
internal/config/ngt.go 100.00% <ø> (+100.00%) ⬆️
internal/config/observability.go 0.00% <0.00%> (ø)
... and 148 more

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 5e6ffac...6931f99. Read the comment docs.

@kpango kpango requested review from rinx and kmrmt July 10, 2020 07:53
charts/vald/values.yaml Outdated Show resolved Hide resolved
internal/net/grpc/client.go Show resolved Hide resolved
internal/config/grpc.go Outdated Show resolved Hide resolved
internal/net/grpc/option.go Show resolved Hide resolved
internal/net/grpc/pool/option.go Show resolved Hide resolved
internal/net/grpc/pool/pool.go Show resolved Hide resolved
charts/vald/templates/_helpers.tpl Outdated Show resolved Hide resolved
@kpango kpango force-pushed the bugfix/gateway-internal-grpc/bugfixes branch from 77ef0b6 to 5023e7d Compare July 10, 2020 08:58
Comment on lines +297 to +300
if nn.Pods == nil {
nodeByName[nodeName].Pods = new(payload.Info_Pods)
}
if nn.Pods.Pods == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if nn.Pods == nil {
nodeByName[nodeName].Pods = new(payload.Info_Pods)
}
if nn.Pods.Pods == nil {
if nn.Pods == nil {
nodeByName[nodeName].Pods = new(payload.Info_Pods)
} else if nn.Pods.Pods == nil {

log.Errorf("error at SearchByID\t%v", err)
if span != nil {
span.SetStatus(trace.StatusCodeNotFound(err.Error()))
}
return nil, status.WrapWithNotFound(fmt.Sprintf("SearchByID API meta %s's uuid not found", metaID), err, req, info.Get())
return nil, status.WrapWithNotFound(fmt.Sprintf("SearchByID API meta %s's uuid not found", req.GetId()), err, req, info.Get())
Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
line is 128 characters (lll)

if span != nil {
span.SetStatus(trace.StatusCodeInvalidArgument(err.Error()))
}
return nil, status.WrapWithInvalidArgument(fmt.Sprintf("Insert API meta datas %v's vector invalid", vec.GetId()), err, info.Get())
Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
line is 132 characters (lll)

if span != nil {
span.SetStatus(trace.StatusCodeInvalidArgument(err.Error()))
}
return nil, status.WrapWithInvalidArgument(fmt.Sprintf("MultiInsert API meta datas %v's vector invalid", vec.GetId()), err, info.Get())
Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
line is 138 characters (lll)

if span != nil {
span.SetStatus(trace.StatusCodeInvalidArgument(err.Error()))
}
return nil, status.WrapWithInvalidArgument(fmt.Sprintf("Update API meta datas %v's vector invalid", vec.GetId()), err, info.Get())
Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
line is 132 characters (lll)

if span != nil {
span.SetStatus(trace.StatusCodeInvalidArgument(err.Error()))
}
return nil, status.WrapWithInvalidArgument(fmt.Sprintf("MultiUpdate API meta datas %v's vector invalid", vec.GetId()), err, info.Get())
Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
line is 138 characters (lll)

if span != nil {
span.SetStatus(trace.StatusCodeInvalidArgument(err.Error()))
}
return nil, status.WrapWithInvalidArgument(fmt.Sprintf("Upsert API meta datas %v's vector invalid", vec.GetId()), err, info.Get())
Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
line is 132 characters (lll)

@@ -290,6 +294,13 @@ func (s *server) Insert(ctx context.Context, vec *payload.Object_Vector) (ce *pa
span.End()
}
}()
if len(vec.GetVector()) < 2 {
err = errors.ErrInvalidDimensionSize(len(vec.GetVector()), 0)
if span != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
if statements should only be cuddled with assignments used in the if statement itself (wsl)

@@ -389,6 +400,13 @@ func (s *server) MultiInsert(ctx context.Context, vecs *payload.Object_Vectors)
metaMap := make(map[string]string)
metas := make([]string, 0, len(vecs.GetVectors()))
for i, vec := range vecs.GetVectors() {
if len(vec.GetVector()) < 2 {
err = errors.ErrInvalidDimensionSize(len(vec.GetVector()), 0)
if span != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
if statements should only be cuddled with assignments used in the if statement itself (wsl)

@@ -389,6 +400,13 @@ func (s *server) MultiInsert(ctx context.Context, vecs *payload.Object_Vectors)
metaMap := make(map[string]string)
metas := make([]string, 0, len(vecs.GetVectors()))
for i, vec := range vecs.GetVectors() {
if len(vec.GetVector()) < 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
mnd: Magic number: 2, in detected (gomnd)

@@ -550,6 +575,13 @@ func (s *server) MultiUpdate(ctx context.Context, vecs *payload.Object_Vectors)
}()
ids := make([]string, 0, len(vecs.GetVectors()))
for _, vec := range vecs.GetVectors() {
if len(vec.GetVector()) < 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

[golangci] reported by reviewdog 🐶
mnd: Magic number: 2, in detected (gomnd)

kevindiu
kevindiu previously approved these changes Jul 10, 2020
Copy link
Contributor

@kevindiu kevindiu left a comment

Choose a reason for hiding this comment

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

LGTM

@kpango kpango force-pushed the bugfix/gateway-internal-grpc/bugfixes branch from 5023e7d to 22cf425 Compare July 10, 2020 09:22
@kpango kpango force-pushed the bugfix/gateway-internal-grpc/bugfixes branch from 22cf425 to 9dcd4ef Compare July 10, 2020 09:26
@github-actions github-actions bot removed the size/L label Jul 10, 2020
@kpango kpango force-pushed the bugfix/gateway-internal-grpc/bugfixes branch from 9dcd4ef to 8c8fcb3 Compare July 10, 2020 09:26
@kpango
Copy link
Collaborator Author

kpango commented Jul 10, 2020

/rebase

@vdaas-ci
Copy link
Collaborator

[REBASE] Rebase triggered by kpango for branch: bugfix/gateway-internal-grpc/bugfixes

Signed-off-by: kpango <i.can.feel.gravity@gmail.com>
@vdaas-ci vdaas-ci force-pushed the bugfix/gateway-internal-grpc/bugfixes branch from 8c8fcb3 to 6931f99 Compare July 10, 2020 09:37
@kpango kpango merged commit 9a2ef4f into master Jul 10, 2020
@kpango kpango deleted the bugfix/gateway-internal-grpc/bugfixes branch July 10, 2020 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants