Skip to content

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

Merged
merged 12 commits into from
Apr 14, 2025
Merged

fix: Cyclotron vars #31178

merged 12 commits into from
Apr 14, 2025

Conversation

benjackwhite
Copy link
Contributor

@benjackwhite benjackwhite commented Apr 14, 2025

Problem

The cyclotron config setup was completely borked due to duplicates all over the place. Attempted to simplify this with stronger typing

Changes

  • Removes unused duplicates
  • Improves how the configs are configured so it should be much clearer now
  • Removes the "bulk copy" setting from the worker config - it seemed to only be used by the janitor tests - the worker never inserts jobs

Does this work well for both Cloud and self-hosted?

How did you test this code?

@benjackwhite benjackwhite marked this pull request as ready for review April 14, 2025 11:00
@benjackwhite benjackwhite requested a review from eli-r-ph April 14, 2025 11:00
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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 and shouldUseBulkJobCopy 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

@benjackwhite benjackwhite requested review from oliverb123 and a team April 14, 2025 11:09
Copy link
Contributor

@oliverb123 oliverb123 left a 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 👍

@benjackwhite benjackwhite merged commit 6cedcf2 into master Apr 14, 2025
94 checks passed
@benjackwhite benjackwhite deleted the fix/cyclotron-configs branch April 14, 2025 12:56
lricoy pushed a commit that referenced this pull request Apr 14, 2025
should_compress_vm_state,
should_use_bulk_job_copy,
);
let manager = QueueManager::from_pool(db.clone(), should_compress_vm_state, false);

Copy link
Contributor

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?

Copy link
Contributor Author

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

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.

3 participants