-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: Cyclotron vars #31178
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
fix: Cyclotron vars #31178
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR refactors the Cyclotron configuration system to eliminate duplicated settings and improve type safety across multiple components.
- Moves
shouldCompressVmState
andshouldUseBulkJobCopy
from individual pool/shard configs to the appropriate root level in both TypeScript and Rust implementations - Updates
@posthog/cyclotron
from version 0.1.10 to 0.1.11 in both package.json files - Adds
#[serde(alias="...")]
attributes in Rust to support camelCase JSON field names for better JS/TS integration - Restructures worker configuration by merging tuning parameters into a single configuration object
- Removes the now-unnecessary
CyclotronWorkerTuningConfig
type while maintaining backward compatibility
10 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks right to me 👍
should_compress_vm_state, | ||
should_use_bulk_job_copy, | ||
); | ||
let manager = QueueManager::from_pool(db.clone(), should_compress_vm_state, false); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 @benjackwhite should the bulk job copy be wired up here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got rid of it as it makes no sense. this is just a test so we dont want to modify our whole worker config to support something that the service itself never actually does
Problem
The cyclotron config setup was completely borked due to duplicates all over the place. Attempted to simplify this with stronger typing
Changes
Does this work well for both Cloud and self-hosted?
How did you test this code?