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

add remove sample code for tutorial #1053

Merged
merged 14 commits into from
Mar 16, 2021

Conversation

vankichi
Copy link
Contributor

@vankichi vankichi commented Mar 2, 2021

Signed-off-by: vankichi [email protected]

Description:

Since #1050 , I added the removing index section for sample codes.
also, updated the doc/tutorial/get-started.md along with above changes.

I already confirmed these sample codes run well in my local environment (docker and k3d).
I appreciate it if you try Get-Started too.

Related Issue:

#1050

How Has This Been Tested?:

Environment:

  • Go Version: 1.16
  • Docker Version: 19.03.8
  • Kubernetes Version: 1.18.2
  • NGT Version: 1.12.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.

@vdaas-ci
Copy link
Collaborator

vdaas-ci commented Mar 2, 2021

[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

example/client/agent/main.go Outdated Show resolved Hide resolved
example/helm/values-scylla.yaml Outdated Show resolved Hide resolved
example/helm/values-scylla.yaml Outdated Show resolved Hide resolved
example/helm/values-scylla.yaml Outdated Show resolved Hide resolved
example/helm/values-scylla.yaml Outdated Show resolved Hide resolved
@github-actions github-actions bot added size/L and removed size/M labels Mar 2, 2021
@vankichi vankichi force-pushed the documentation/docs/add-remove-index-sec-for-tutorial branch from acbd033 to 1a89537 Compare March 2, 2021 09:32
@vankichi vankichi marked this pull request as ready for review March 2, 2021 09:37
@vankichi vankichi changed the title [WIP] add remove code add remove sample code for tutorial Mar 2, 2021


- Remove manually instead of waiting for auto indexing.
If you run below code, the indexes will be removed from the Vald Agent and the Backup file.
Copy link
Collaborator

@hlts2 hlts2 Mar 3, 2021

Choose a reason for hiding this comment

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

I through the eyes of the user, I thought that CreateIndex would mean deleting the file, so what about the following?. if I made a mistake, I am sorry...

Suggested change
If you run below code, the indexes will be removed from the Vald Agent and the Backup file.
If you run below code, the indexes will be removed from the Vald Agent and the Backup file because you deleted all the inserted vectors in the above section.

Copy link
Collaborator

@hlts2 hlts2 Mar 3, 2021

Choose a reason for hiding this comment

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

btw, Should we change from SaveIndex to SaveAndCreateIndex method.
Will the backup file be erased, but will the indexing data be erased as well? 🤔
I made a mistake, I am sorry...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hlts2 @kevindiu
I think CreateAndSaveIndex() won't work well.
cuz, there is no index to create and save.
I just tried to use CreateAndSaveIndex() instead of the current implementation, but end with the error FailedPreCondition desc = CreateAndSaveIndex API failed to create indexes pool_size = 400.
If you can, could you please try it?

Copy link
Contributor Author

@vankichi vankichi Mar 9, 2021

Choose a reason for hiding this comment

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

@hlts2 @kevindiu
I should announce to guys that the current vald-agent=ngt:latest image will not work well, so if you'd like to confirm the tutorial, please use the nightly version.
The latest version will be updated when released next version.

And also, CreateAndSaveIndex() is good. but, this time, I'd like to use another method as SaveIndex() instead of this to show vald client does not have only CreateAndSaveIndex().
How do you think about it?

example/helm/values-scylla.yaml Outdated Show resolved Hide resolved
docs/tutorial/get-started.md Outdated Show resolved Hide resolved
@vankichi vankichi requested review from kevindiu and hlts2 March 10, 2021 04:50
kevindiu
kevindiu previously approved these changes Mar 15, 2021
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

hlts2
hlts2 previously approved these changes Mar 16, 2021
Copy link
Collaborator

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

LGTM

@kevindiu
Copy link
Contributor

/rebase
/format
/approve

@vdaas-ci
Copy link
Collaborator

[REBASE] Rebase triggered by kevindiu for branch: documentation/docs/add-remove-index-sec-for-tutorial

@vdaas-ci vdaas-ci dismissed stale reviews from hlts2 and kevindiu via 9ae9ae3 March 16, 2021 01:26
@vdaas-ci vdaas-ci force-pushed the documentation/docs/add-remove-index-sec-for-tutorial branch from 32685bd to 9ae9ae3 Compare March 16, 2021 01:26
@vdaas-ci
Copy link
Collaborator

[FORMAT] Updating license headers and formatting go codes triggered by kevindiu.

Copy link
Collaborator

@vdaas-ci vdaas-ci left a comment

Choose a reason for hiding this comment

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

[APPROVED] This PR is approved by kevindiu.

@kevindiu kevindiu merged commit 777c96e into master Mar 16, 2021
@kevindiu kevindiu deleted the documentation/docs/add-remove-index-sec-for-tutorial branch March 16, 2021 01:50
@vdaas-ci vdaas-ci mentioned this pull request Mar 17, 2021
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.

4 participants