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.

Sign up for GitHub

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

Jump to bottom

Add vald standard internal/sync package #2153

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Add vald standard internal/sync package #2153

merged 1 commit into from
Aug 31, 2023

Conversation

kpango
Copy link
Collaborator

@kpango kpango commented Aug 15, 2023 •
edited
Loading

Description:

Refactor around Vald internal/sync.

The following are the details of the implementation.

  1. internal/errgroup was moved to internal/sync/errgroup.
  2. Internal/sync/errgroup implementation now blocks Go func dispatches if goroutine limit is set.
  3. internal/singleflight has been moved to internal/sync/singleflight
  4. internal/sync/singleflight has been changed to an implementation using Generics, and bugs caused by RaceCondition in previous versions have been resolved.
  5. new internal/sync/semaphore has been created.
  6. The valdsync alias package import, which was used to avoid conflicts with standard sync packages, has been removed and internal/sync/alias.go now contains aliases for all sync packages.
  7. changed the license description of packages that have been modified based on the official Go language implementation to the correct one.
  8. modified hack/license/gen to implement 6.
  9. internal/safety package has eliminated dependencies on runtime/debug packages, which reduces the possibility of panics and switches stack traces to Vald Official ones.
  10. pkg/agent/core/ngt/model has been removed and is now directly internally converted to gRPC Search_Response payloads in Vald.
  11. fixed DataRace in tests/e2e/operation/stream.go
  12. update of Go modules and Libraries

Please ignore apis, versions package and _test.go changes which is just re-generated by Vald Makefile functionalities.

Related Issue:

Versions:

  • Go Version: 1.21.0
  • Docker Version: 20.10.8
  • Kubernetes Version: v1.27.3
  • NGT Version: 2.0.16

Checklist:

Special notes for your reviewer:

@vdaas-ci
Copy link
Collaborator

[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 main
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

github-actions[bot]
github-actions bot reviewed Aug 15, 2023
View reviewed changes
internal/sync/singleflight/singleflight_test.go Show resolved Hide resolved
internal/sync/singleflight/singleflight.go Outdated Show resolved Hide resolved
internal/client/v1/client/discoverer/discover.go Outdated Show resolved Hide resolved
internal/sync/singleflight/singleflight_test.go Show resolved Hide resolved
internal/sync/singleflight/singleflight_test.go Show resolved Hide resolved
internal/sync/errgroup/errgroup_test.go Show resolved Hide resolved
internal/sync/semaphore/semaphore_bench_test.go Show resolved Hide resolved
internal/sync/semaphore/semaphore_bench_test.go Show resolved Hide resolved
internal/sync/singleflight/singleflight_test.go Outdated Show resolved Hide resolved
internal/sync/singleflight/singleflight_test.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 15, 2023 •
edited
Loading

Codecov Report

Patch coverage is 74.93% of modified lines.

Files Changed Coverage
hack/benchmark/internal/operation/insert.go ø
hack/benchmark/internal/operation/remove.go ø
hack/benchmark/internal/operation/search.go ø
...k/benchmark/internal/starter/agent/core/ngt/ngt.go ø
hack/license/gen/main.go 0.00%
internal/backoff/backoff.go ø
internal/circuitbreaker/manager.go ø
internal/client/v1/client/discoverer/discover.go 0.00%
internal/client/v1/client/discoverer/option.go ø
internal/client/v1/client/filter/egress/client.go ø
... and 87 more

📢 Thoughts on this report? Let us know!.

@kpango kpango force-pushed the refactor/internal-sync/add-vald-std-sync-package branch from ef061be to 4a93f18 Compare August 18, 2023 01:50
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Aug 18, 2023 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2a0d587
Status: ✅  Deploy successful!
Preview URL: https://2208a760.vald.pages.dev
Branch Preview URL: https://refactor-internal-sync-add-v.vald.pages.dev

View logs

@kpango kpango force-pushed the refactor/internal-sync/add-vald-std-sync-package branch from 4a93f18 to 1ac0836 Compare August 18, 2023 02:01
@github-actions
Copy link
Contributor

[FOSSA] The scan result will be available at https://app.fossa.com/projects/custom%2B21465%2Fvald/refs/branch/refactor%2Finternal-sync%2Fadd-vald-std-sync-package/b7b7fd6a483f973681ad6c4dc174ba910b42fe06

@vdaas-ci
Copy link
Collaborator

Profile Report

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

@kpango kpango force-pushed the refactor/internal-sync/add-vald-std-sync-package branch from 1ac0836 to fb5cebc Compare August 25, 2023 03:11
@kpango kpango force-pushed the refactor/internal-sync/add-vald-std-sync-package branch from fb5cebc to 2be848a Compare August 25, 2023 03:35
@kpango kpango force-pushed the refactor/internal-sync/add-vald-std-sync-package branch from c7feeb7 to d321745 Compare August 27, 2023 17:51
@kpango kpango force-pushed the refactor/internal-sync/add-vald-std-sync-package branch 2 times, most recently from 4d884c9 to 6ab906f Compare August 29, 2023 04:14
@kpango kpango changed the title [WIP] add vald standard internal/sync package Add vald standard internal/sync package Aug 29, 2023
@github-actions
Copy link
Contributor

[FOSSA] The scan result will be available at https://app.fossa.com/projects/custom%2B21465%2Fvald/refs/branch/refactor%2Finternal-sync%2Fadd-vald-std-sync-package/141d2329cb76f20dd958cb29797ee5514d7248dc

@vdaas-ci
Copy link
Collaborator

Profile Report

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

kpango
kpango commented Aug 30, 2023
View reviewed changes
Copy link
Collaborator Author

@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.

Self checked

@kpango kpango requested review from vankichi and hlts2 August 30, 2023 01:31
@kpango kpango force-pushed the refactor/internal-sync/add-vald-std-sync-package branch from 6ab906f to 2a0d587 Compare August 30, 2023 01:58
hlts2
hlts2 approved these changes Aug 30, 2023
View reviewed changes
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!

vankichi
vankichi approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@vankichi vankichi left a comment

Choose a reason for hiding this comment

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

LGTM

@vankichi vankichi merged commit 5689309 into main Aug 31, 2023
86 of 88 checks passed
@vankichi vankichi deleted the refactor/internal-sync/add-vald-std-sync-package branch August 31, 2023 01:12
@hlts2 hlts2 mentioned this pull request Sep 13, 2023
Merged
ykadowak added a commit that referenced this pull request Nov 30, 2023
@ykadowak
Revert "Add Vald standard internal/sync package (#2153)"
336d292 
This reverts commit 5689309.
ykadowak added a commit that referenced this pull request Nov 30, 2023
@ykadowak
Revert "Add Vald standard internal/sync package (#2153)"
25493cf 
This reverts commit 5689309.
ykadowak added a commit that referenced this pull request Nov 30, 2023
@ykadowak
Revert "Add Vald standard internal/sync package (#2153)"
21015bb 
This reverts commit 5689309.
ykadowak added a commit that referenced this pull request Nov 30, 2023
@ykadowak
Revert "Add Vald standard internal/sync package (#2153)"
31cee29 
This reverts commit 5689309.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@hlts2 hlts2 hlts2 approved these changes

@vankichi vankichi vankichi approved these changes

Assignees
No one assigned
Labels
actions/e2e-chaos actions/e2e-deploy actions/e2e-max-dim actions/e2e-profiling actions/fossa area/agent/core area/agent/sidecar area/discoverer area/gateway/filter area/gateway/lb area/internal area/makefile area/manager/index area/tools/cli/loadtest priority/low size/XXXL team/set SET team team/sre SRE team type/refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kpango @vdaas-ci @vankichi @hlts2