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

implement pkg handler remove test cases #1644

Merged
merged 8 commits into from
May 17, 2022

Conversation

kmrmt
Copy link
Contributor

@kmrmt kmrmt commented May 9, 2022

Signed-off-by: Kosuke Morimoto ksk@vdaas.org

Description:

SSIA

Related Issue:

How Has This Been Tested?:

Environment:

  • Go Version: 1.18.1
  • Docker Version: 20.10.8
  • Kubernetes Version: 1.22.0
  • NGT Version: 1.14.3

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.

Signed-off-by: Kosuke Morimoto <ksk@vdaas.org>
@kmrmt kmrmt requested review from kevindiu and vankichi May 9, 2022 09:37
@vdaas-ci
Copy link
Collaborator

vdaas-ci commented May 9, 2022

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 💌 /changelog - replace the PR body by changelog details
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • /rebase - rebase master
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #1644 (a7bf98d) into master (13c62b1) will increase coverage by 0.10%.
The diff coverage is 43.29%.

@@            Coverage Diff             @@
##           master    #1644      +/-   ##
==========================================
+ Coverage   30.70%   30.81%   +0.10%     
==========================================
  Files         372      372              
  Lines       32234    32395     +161     
==========================================
+ Hits         9897     9981      +84     
- Misses      21970    22040      +70     
- Partials      367      374       +7     
Impacted Files Coverage Δ
pkg/agent/core/ngt/handler/grpc/handler.go 14.04% <42.23%> (+3.36%) ⬆️
internal/errors/ngt.go 89.65% <100.00%> (+0.56%) ⬆️
...nt/core/ngt/service/vqueue/uninserted_index_map.go 70.49% <0.00%> (-5.47%) ⬇️
internal/backoff/backoff.go 85.54% <0.00%> (-2.41%) ⬇️
internal/worker/worker.go 84.37% <0.00%> (+0.78%) ⬆️
internal/worker/queue.go 100.00% <0.00%> (+1.26%) ⬆️
internal/errgroup/group.go 95.00% <0.00%> (+2.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 6d6a6c4...a7bf98d. Read the comment docs.

@vankichi
Copy link
Contributor

@kmrmt
the case is fine :)
please keep continuing implementation!

Signed-off-by: Kosuke Morimoto <ksk@vdaas.org>
@github-actions github-actions bot added size/L and removed size/S labels May 10, 2022
@kmrmt kmrmt changed the title [WIP] implement pkg handler remove test cases implement pkg handler remove test cases May 10, 2022
@kmrmt kmrmt marked this pull request as ready for review May 10, 2022 16:09
kmrmt added 2 commits May 11, 2022 10:00
Signed-off-by: Kosuke Morimoto <ksk@vdaas.org>
Signed-off-by: Kosuke Morimoto <ksk@vdaas.org>
kmrmt added 2 commits May 11, 2022 16:25
Signed-off-by: Kosuke Morimoto <ksk@vdaas.org>
Signed-off-by: Kosuke Morimoto <ksk@vdaas.org>
@kmrmt kmrmt requested a review from kpango May 11, 2022 09:23
Co-authored-by: Yusuke Kato <kpango@vdaas.org>
Comment on lines +4627 to +4628
indexId string
removeId string
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
indexId string
removeId string
originId string
indexedId string

or

Suggested change
indexId string
removeId string
indexId string
targetId string

umm... 🤔
Of course, I do not care if you won't change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intent is that indexId means the id which is in the Vald and removeId means the id which is request for remove.
It is too difficult so let me put it in current expression for once.

@kevindiu
Copy link
Contributor

kevindiu commented May 12, 2022

I think this changes needs to pass E2E, what do you think?

@vdaas-ci
Copy link
Collaborator

Profile Report

typevald-agent-ngtvald-lb-gatewayvald-discoverervald-manager-index
cpu
heap
other images

@vankichi
Copy link
Contributor

@kpango CRE review has passed. Please review.

Copy link
Collaborator

@kpango kpango 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 merged commit d55e2db into master May 17, 2022
@kpango kpango deleted the test/pkg/implement-agent-ngt-handler-remove-test branch May 17, 2022 08:00
This was referenced May 24, 2022
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