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

Distribute cron tasks #3326

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Distribute cron tasks #3326

wants to merge 33 commits into from

Conversation

tgregory-block
Copy link
Collaborator

Currently Cron tasks are governed by a single lease, so all tasks are executed on whichever pod takes that lease. This change adds a new CronLeaseBehavior parameter to CronModule, making it possible to take out a lease per-cron-task and distribute the tasks across the deployment. The default behavior remains to take out a single lease

keeferrourke
keeferrourke reviewed Jun 24, 2024
View reviewed changes
misk-cron/src/main/kotlin/misk/cron/CronManager.kt Outdated
}

internal fun buildTaskLease(klass: KClass<out Runnable>): Lease {
val sanitizedClassName = klass.qualifiedName!!.replace(".", "-")
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are some gotchas here with inner/anonymous classes. You might end up with some very strange looking lease names. Is this replacement just for readability or are there actual characters/sequences you intend to avoid in the lease name string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We mainly weren't sure if the partition key had forbidden characters, and since the dynamo lease implementation lives in Skim it wasn't straightforward to write an integration test. Looking more thoroughly at the dynamo docs, it doesn't seem like that's the case though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the anonymous object case, the existing implementation also force unwraps the runnable class's qualifiedName, so we're not breaking something that was previously possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the sanitization, . are clearly fine since the original cluster wide lease uses them

mhickman
mhickman reviewed Jun 25, 2024
View reviewed changes
misk-cron/src/main/kotlin/misk/cron/CronManager.kt
Comment on lines +102 to +103
CronLeaseBehavior.ONE_LEASE_PER_CRON -> tryAcquireLeasePerCronAndRunCrons(lastRun)
CronLeaseBehavior.ONE_LEASE_PER_CLUSTER -> tryAcquireSingleLeaseAndRunCrons(lastRun)
Copy link
Collaborator

Choose a reason for hiding this comment

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

worth adding any checks + behavior for rolling this out? migrating from per-cluster to per-cron means there'll likely be overlaps during a deploy

tgregory-block and others added 23 commits August 20, 2024 16:21
@tgregory-block
Acquire a lease per cron task
fe0c327
@din-cashapp @tgregory-block
Add backwards compatibility and update README
2a41ba9
@din-cashapp @tgregory-block
resolve comments
4b0b4f9
@tgregory-block
API dump
7aa7afe
@din-cashapp @tgregory-block
fixes
7b7a0a6
@tgregory-block
Remove unnecessary class name sanitization
cdf5d07
@tgregory-block
Lease held elsewhere WIP
4c8d547
@mmollaverdi @tgregory-block
Implement TestFixture for log collectors
7c32ffd 
GitOrigin-RevId: 453df97428102c5f1959afd94c83d52623e8387c
@mmollaverdi @tgregory-block
Implement TestFixture for metrics
8f5b240 
GitOrigin-RevId: c51ec0fb5dcc67f9e1b6b2564678fd86d3f3c416
@mmollaverdi @tgregory-block
Implement TestFixture for JdbcTestingModule
7c600c8 
GitOrigin-RevId: 611be8a7a60a1decf8ea787aca23dacf01c1224a
@damar-block @tgregory-block
Support more source formats and Guice explorer
93c6b21 
GitOrigin-RevId: b6a255f455c40ef5e21059ab5582a939be572b35
@salamagd @tgregory-block
Updates the hashing algorithm used in the ClusterHashRing
3cdb3e4 
implementation of `ClusterResourceMapper` from `murmur3_32` in the
`com.google.common.hash.Hashing` package, to `murmur3_32_fixed`, which
addresses a bug when generating a hashed value from a string containing
non-BMP characters.

GitOrigin-RevId: f1d4efcd61eb40270b838221bf8a0fc1bca38b1d
@mmollaverdi @tgregory-block
Implement TestFixture for dynamodb
f95301e 
GitOrigin-RevId: 43385fdc20d5036dd27002569f988ad76328c3e0
@katukota @tgregory-block
Expose service graph in admin console using d3 graph template
f49a286 
from https://observablehq.com/@d3/mobile-patent-suits?intent=fork

GitOrigin-RevId: 02456e0997b5e0a5c4c5dd2229edc0326bdabfc1
@tyiu @tgregory-block
Fix InProcessDynamoDbTests for Apple Silicon
30a98f9 
GitOrigin-RevId: 294c578de959770de4a3b1de8d99c5b122b85984
@ean5533 @tgregory-block
Update misk.feature.Attributes to have a with method that
3003e10 
returns a misk class rather than inheriting one that returns a wisp
class.

Previously, if someone with a misk.feature.Attribute called `.with`,
they would get back a wisp class which would be incompatible with the
rest of the misk.feature.* classes they were using.

GitOrigin-RevId: 7c5181263816f14e27fdaf36b4287840119d5ce4
@kevink-sq @tgregory-block
Defer the shutdown of JettyService until after Guava managed
2bdbd1a 
services are shutdown by default

GitOrigin-RevId: bce36d20af9ef0221f5179e4c7382854e3d7e04d
@rainecp @tgregory-block
feat: Add new param to Redis modules for specific config
c8bafb6 
Currently, the
[RedisModule](https://github.com/cashapp/misk/blob/8711bf7a6f68ed7b31237d37d811649d958486f2/misk-redis/src/main/kotlin/misk/redis/RedisModule.kt#L74)
in Misk uses a
[RedisConfig](https://github.com/cashapp/misk/blob/bdb3d56f8a418fa630433a6a8e374405c5a21f1e/misk-redis/src/main/kotlin/misk/redis/RedisConfig.kt#L7)
map where the Jedis client is initialized using the first item in the
map. The map is ordered by insertion which is based on the order of how
configurations are loaded. This setup can lead to unintended
consequences, particularly when multiple Redis clusters are provisioned
for a single environment. This adds a new constructor in `RedisModule`
and `RedisClusterModule` to improve config management by allowing
consumers to provide a specific config to use instead of letting the
module choose from a given configuration map.

GitOrigin-RevId: 5175feb11d51f78f77b723dee9943741bf59b8fd
@afkelsall @tgregory-block
The original implementation of TaggedLogger became heavy, stateful and
0b6ac89 
opinionated. We've reconsidered this approach and
simplified it to just a function call `withSmartTags` doing away
with the stateful builders etc.

COPYBARA_INTEGRATE_REVIEW=#3366 from
GitOrigin-RevId: 508c16fd4046947e33c3e282533841978ef2d1a8
@lakpic @tgregory-block
In this approach, we rebuild the url with path and parse it so that
6b15723 
query parameters are extracted.
Advantage of this approach is that tests can pass the full url with
queryparameters (e.g /get?param=value). Disadvantage is that this
changes how we were constructing the url and there is a chance for
breaking old behavior.

GitOrigin-RevId: c5a36bd86576979ac1840f36e6ace3a1c130ed25
@mmollaverdi @tgregory-block
Skip resetting DB if DataSourceService has not started
988a576 
GitOrigin-RevId: efb52773373ea3687903bc3fb2825abc36c595bd
@kevink-sq @tgregory-block
Update MiskTestExtension to stop Jetty in the shutdown hook
2dd10bb 
GitOrigin-RevId: 2d221e51cf17596ec6849d0f2dd4d015df83fe87
@mmollaverdi @tgregory-block
Injector reuse - support test modules with constructor
c10e0fe 
arguments

GitOrigin-RevId: 05141a51cfaa6cb608383b5aa7d163b72347b114
damar-block and others added 10 commits August 20, 2024 16:22
@damar-block @tgregory-block
We are not using DefaultBindingScopingVisitor's visitOther
9cd88a9 
function, so we can just implement the interface directly

GitOrigin-RevId: e52510622590f1068de635b97cc73fc4e7a03dac
@mmollaverdi @tgregory-block
Fixes some misk test reuse injector bugs
75ef2b0 
GitOrigin-RevId: 5c531499ac0f3028ff74d4efbbfceb1344ea7b46
@damar-block @tgregory-block
Add Mutable AI badge to Misk readme
7adc972 
GitOrigin-RevId: 22e9c9a4c3bb23f3874d7a1a8dac1bb37df5443d
@katukota @tgregory-block
[Misk Test Flakes] misk.jdbc.CockroachDbRealTransacterTest
9ac94ab 
GitOrigin-RevId: 739c0b99ae9c919d193c972af82e31e23385f04e
@yissachar @tgregory-block
Only run buildMiskWeb on shadowJar
a722cfb 
GitOrigin-RevId: 509e8755c63c32910d74beab22ecaacd6935d5fa
@mmollaverdi @tgregory-block
Misk test injector reuse - dynamodb reset fix and mockito test
7744ee3 
fixture

GitOrigin-RevId: 8164ac1d5ed2ce86690a377921b05873aedf4a4e
@yissachar @tgregory-block
Build misk-web on assemble, not just shadowJar
d705db1 
GitOrigin-RevId: 489b1ed32a14f269a03f2089c9f577679b46be7c
@yissachar @tgregory-block
Revert only building misk-web on assemble
2d2ec8b 
GitOrigin-RevId: 23e05e36ea6d3f06c66d25b51b2af5f4eea4c65b
@katukota @tgregory-block
Fix flaky test
99f639a 
GitOrigin-RevId: 034d8309a3b9b7d471f900134a51f077093ef74c
@meghans-cash @tgregory-block
Add flush_interval to the LaunchDarklyConfig, default to the
6d42628 
default flush interval to allow configuring how often events are sent

GitOrigin-RevId: 5e36390bc9fe80da834a7b8baf036591e85536fa
@tgregory-block tgregory-block force-pushed the tgregory/distribute-cron-tasks branch from 0461ab6 to 6d42628 Compare August 20, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhickman mhickman mhickman left review comments

@frojasg frojasg Awaiting requested review from frojasg

@adrw adrw Awaiting requested review from adrw

@din-cashapp din-cashapp Awaiting requested review from din-cashapp

@keeferrourke keeferrourke Awaiting requested review from keeferrourke

At least 5 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.