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 gache's generic Map as internal/sync.Map and replace standard sync.Map with it #2115

Merged
merged 13 commits into from
Jul 11, 2023
Merged

Add gache's generic Map as internal/sync.Map and replace standard sync.Map with it #2115

merged 13 commits into from
Jul 11, 2023

Conversation

ykadowak
Copy link
Contributor

@ykadowak ykadowak commented Jul 10, 2023 •
edited by vankichi
Loading

Description:

SSIA

  • Namespace sync is taken by the standard library so used alias valdsync, which doesn't look so good to me but couldn't come up with a better name.
  • Replace all the expunges except grpcconns which has a original Range logic so it wasn't possible to replace with valdsync.Map

Related Issue:

Versions:

  • Go Version: 1.20.5
  • Docker Version: 20.10.8
  • Kubernetes Version: v1.27.3
  • NGT Version: 2.0.13

Checklist:

Special notes for your reviewer:

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 10, 2023 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6bac855
Status: ✅  Deploy successful!
Preview URL: https://a09828de.vald.pages.dev
Branch Preview URL: https://feature-internal-sync-map-2.vald.pages.dev

View logs

@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

@ykadowak ykadowak force-pushed the feature/internal/sync-map-2 branch from 01d1f93 to 0b6c427 Compare July 10, 2023 05:39
@ykadowak ykadowak mentioned this pull request Jul 10, 2023
Closed
2 tasks
@ykadowak ykadowak changed the title [WIP] Feature/internal/sync map 2 Feature/internal/sync map 2 Jul 10, 2023
@ykadowak ykadowak changed the title Feature/internal/sync map 2 Add gache generic Map as internal/sync.Map and replace standard sync.Map with it Jul 10, 2023
@ykadowak ykadowak changed the title Add gache generic Map as internal/sync.Map and replace standard sync.Map with it Add gache's generic Map as internal/sync.Map and replace standard sync.Map with it Jul 10, 2023
@ykadowak ykadowak requested review from a team, vankichi and datelier and removed request for a team, vankichi and datelier July 10, 2023 06:55
@ykadowak ykadowak changed the title Add gache's generic Map as internal/sync.Map and replace standard sync.Map with it [WIP] Add gache's generic Map as internal/sync.Map and replace standard sync.Map with it Jul 10, 2023
@codecov
Copy link

codecov bot commented Jul 10, 2023 •
edited
Loading

Codecov Report

Patch coverage: 88.46% and project coverage change: +0.45 🎉

Comparison is base (a6d8701) 30.65% compared to head (342d3b8) 31.11%.

❗ Current head 342d3b8 differs from pull request most recent head 6bac855. Consider uploading reports for the commit 6bac855 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2115      +/-   ##
==========================================
+ Coverage   30.65%   31.11%   +0.45%     
==========================================
  Files         347      339       -8     
  Lines       34255    32793    -1462     
==========================================
- Hits        10501    10202     -299     
+ Misses      23276    22140    -1136     
+ Partials      478      451      -27
Impacted Files Coverage Δ
internal/cache/cacher/cacher.go 100.00% <ø> (ø)
internal/circuitbreaker/manager.go 0.00% <0.00%> (ø)
internal/client/v1/client/discoverer/discover.go 0.00% <0.00%> (ø)
internal/client/v1/client/filter/egress/client.go 0.00% <ø> (ø)
internal/client/v1/client/filter/ingress/client.go 0.00% <ø> (ø)
internal/net/grpc/client.go 0.00% <0.00%> (ø)
pkg/agent/core/ngt/service/vqueue/queue.go 72.78% <ø> (ø)
pkg/discoverer/k8s/service/discover.go 0.00% <ø> (ø)
pkg/gateway/lb/handler/grpc/aggregation.go 0.00% <ø> (ø)
pkg/gateway/lb/service/gateway.go 0.00% <0.00%> (ø)
... and 8 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions github-actions bot removed the size/M label Jul 10, 2023
kpango
kpango previously approved these changes Jul 11, 2023
View reviewed changes
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

kevindiu
kevindiu previously approved these changes Jul 11, 2023
View reviewed changes
vankichi
vankichi previously approved these changes Jul 11, 2023
View reviewed changes
@ykadowak ykadowak dismissed stale reviews from vankichi, kevindiu, and kpango via 6bac855 July 11, 2023 08:15
@vdaas-ci
Copy link
Collaborator

Profile Report

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

@vdaas-ci
Copy link
Collaborator

Profile Report

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

@vankichi vankichi merged commit e52eb4a into main Jul 11, 2023
141 of 144 checks passed
@vankichi vankichi deleted the feature/internal/sync-map-2 branch July 11, 2023 08:46
@vdaas-ci
Copy link
Collaborator

Profile Report

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

@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 gache's generic Map as internal/sync.Map and replace stan…
b6af2d2 
…dard sync.Map with it (#2115)"

This reverts commit e52eb4a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions GitHub Actions GitHub Actions left review comments

@kevindiu kevindiu kevindiu approved these changes

@hlts2 hlts2 hlts2 approved these changes

@kpango kpango kpango left review comments

@vankichi vankichi vankichi left review comments

@kmrmt kmrmt Awaiting requested review from kmrmt kmrmt was automatically assigned from vdaas/maintainer

Assignees
No one assigned
Labels
actions/e2e-chaos actions/e2e-deploy actions/e2e-max-dim actions/e2e-profiling area/agent/core area/discoverer area/gateway/lb area/internal area/manager/index priority/medium size/XXXL team/set SET team type/feature New feature
Projects
None yet
Milestone
No milestone
Development

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