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

chore(infra): sst ebs option #3856

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

chore(infra): sst ebs option #3856

wants to merge 3 commits into from

Conversation

cesara
Copy link
Collaborator

@cesara cesara commented Feb 22, 2025

No description provided.

Copy link

vercel bot commented Feb 22, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
replicache-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 24, 2025 10:16pm
zbugs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 24, 2025 10:16pm

Copy link

github-actions bot commented Feb 22, 2025

🐰 Bencher Report

Branchcesara/ebs_sst
Testbedlocalhost
Click to view all benchmark results
BenchmarkFile SizeBenchmark Result
kilobytes (KB)
(Result Δ%)
Upper Boundary
kilobytes (KB)
(Limit %)
zero-package.tgz📈 view plot
🚷 view threshold
969.81 KB
(+0.04%)Baseline: 969.38 KB
988.77 KB
(98.08%)
zero.js📈 view plot
🚷 view threshold
180.29 KB
(0.00%)Baseline: 180.29 KB
183.89 KB
(98.04%)
zero.js.br📈 view plot
🚷 view threshold
50.10 KB
(-0.04%)Baseline: 50.12 KB
51.12 KB
(98.00%)
🐰 View full continuous benchmarking report in Bencher

Copy link

github-actions bot commented Feb 22, 2025

🐰 Bencher Report

Branchcesara/ebs_sst
Testbedlocalhost
Click to view all benchmark results
BenchmarkThroughputBenchmark Result
operations / second (ops/s)
(Result Δ%)
Lower Boundary
operations / second (ops/s)
(Limit %)
src/client/zero.bench.ts > basics > All 1000 rows x 10 columns (numbers)📈 view plot
🚷 view threshold
75.82 ops/s
(+0.37%)Baseline: 75.54 ops/s
70.31 ops/s
(92.73%)
src/client/zero.bench.ts > with filter > Lower rows 500 x 10 columns (numbers)📈 view plot
🚷 view threshold
101.88 ops/s
(+8.72%)Baseline: 93.71 ops/s
90.80 ops/s
(89.13%)
🐰 View full continuous benchmarking report in Bencher

Copy link
Contributor

@darkgnotic darkgnotic left a comment

Choose a reason for hiding this comment

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

So awesome to see this all come together. Nice work going under the covers to get the ebs stuff defined (without it being officially supported by sst).


const addEbsVolumeConfig = (
transform: any,
ecsVolumeRole: aws.iam.Role | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a lambda within the lexical scope of ecsVolumeRole, and the latter is essentially constant, I think you can just reference it instead of passing it in (thereby simplifying the callers).

) => {
return {
...transform,
service: IS_EBS_STAGE
Copy link
Contributor

Choose a reason for hiding this comment

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

I see how it's kind of tricky to recursively merge the stuff from the ebs volume.

However, we might be able to simplify this with the defu library which essentially does a recursive merge (we use this in our flags parser).

Although I don't think it will "merge" the taskDefinition function, we only define that once so I think it will do the right thing.

So ideally it can look something like:

const EBS_TRANSFORM = !IS_EBS_STAGE ? {} : {
  service: {
    volumneConfiguration: { ... },
  },
  taskDefinition: (args: any) => { ... },
};


// Then in the actual service definition:
const replicationManager = cluster.addService(`replication-manager`, {
  ...
  transform: defu(EBS_TRANSFORM, {
    service: {
      healthCheckGracePeriodSeconds: 300, // same as health.startPeriod
    },
    ....
  }),
});

Can you see if this would work? I think it would be make this easier to read and maintain.

healthyThreshold: 2,
timeout: 3,
},
...BASE_TRANSFORM.target,
Copy link
Contributor

@darkgnotic darkgnotic Feb 24, 2025

Choose a reason for hiding this comment

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

Does this work?

transform: defu(
  {
    target: {
      stickiness: { ... }
    }
  },
  EBS_TRANSFORM,
  BASE_TRANSFORM,
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants