Closed davidporter-id-au closed 4 days ago
codecov[bot]
commented
3 weeks ago Attention: Patch coverage is 91.60305% with 11 lines in your changes missing coverage. Please review.
Project coverage is 72.10%. Comparing base (
34cfbb3) to head (13bfc7e).:exclamation: Current head 13bfc7e differs from pull request most recent head 2abd7e5
Please upload reports for the commit 2abd7e5 to get more accurate results.
coveralls
commented
3 weeks ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| common/persistence/persistence-tests/persistenceTestBase.go | 0 | 2 | 0.0% | ||
| service/history/handler/handler.go | 0 | 2 | 0.0% | ||
| service/history/task/transfer_active_task_executor.go | 36 | 38 | 94.74% | ||
| common/persistence/persistence-tests/shardPersistenceTest.go | 0 | 10 | 0.0% | ||
| <!-- | Total: | 118 | 134 | 88.06% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| service/history/task/transfer_standby_task_executor.go | 2 | 86.94% | ||
| common/task/parallel_task_processor.go | 2 | 93.06% | ||
| service/history/replication/task_processor.go | 2 | 82.76% | ||
| common/util.go | 2 | 91.84% | ||
| service/matching/tasklist/task_writer.go | 2 | 82.21% | ||
| common/persistence/metered.go | 2 | 80.87% | ||
| service/history/task/fetcher.go | 2 | 83.13% | ||
| service/history/execution/mutable_state_builder.go | 3 | 78.26% | ||
| service/history/handler/handler.go | 4 | 96.43% | ||
| common/persistence/wrappers/errorinjectors/utils.go | 6 | 91.41% | ||
| <!-- | Total: | 309 | --> |
| Totals | |
|---|---|
| Change from base Build 018fedb8-ecd7-4675-ba4a-3dd7f0818e3a: | -0.1% |
| Covered Lines: | 103817 |
| Relevant Lines: | 145939 |
coveralls
commented
2 weeks ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| common/persistence/persistence-tests/persistenceTestBase.go | 0 | 2 | 0.0% | ||
| service/history/handler/handler.go | 0 | 2 | 0.0% | ||
| service/history/task/transfer_active_task_executor.go | 36 | 38 | 94.74% | ||
| common/persistence/persistence-tests/shardPersistenceTest.go | 0 | 10 | 0.0% | ||
| <!-- | Total: | 118 | 134 | 88.06% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| common/task/weighted_round_robin_task_scheduler.go | 1 | 88.06% | ||
| service/matching/tasklist/db.go | 2 | 73.23% | ||
| service/history/replication/task_processor.go | 2 | 82.76% | ||
| common/util.go | 2 | 91.84% | ||
| common/persistence/metered.go | 2 | 80.87% | ||
| common/persistence/historyManager.go | 2 | 66.67% | ||
| common/log/tag/tags.go | 3 | 50.46% | ||
| common/persistence/nosql/nosql_task_store.go | 3 | 85.52% | ||
| common/task/fifo_task_scheduler.go | 3 | 84.54% | ||
| service/history/execution/mutable_state_builder.go | 3 | 78.26% | ||
| <!-- | Total: | 341 | --> |
| Totals | |
|---|---|
| Change from base Build 018fedb8-ecd7-4675-ba4a-3dd7f0818e3a: | -0.1% |
| Covered Lines: | 103640 |
| Relevant Lines: | 145776 |
davidporter-id-au
commented
1 week ago I think it should be safe to delete. If there is still some dangling code related to the cross-cluster feature, it should be safe to clean up afterward.
appreciate the review, it's annoyingly large. Yeah, this is basically roughly only half of the changes, I've not touched anything in persistence yet. I expect there'll be a few other bits dangling as well
coveralls
commented
5 days ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| common/persistence/persistence-tests/persistenceTestBase.go | 0 | 2 | 0.0% | ||
| service/history/handler/handler.go | 0 | 2 | 0.0% | ||
| service/history/task/transfer_active_task_executor.go | 36 | 38 | 94.74% | ||
| common/persistence/persistence-tests/shardPersistenceTest.go | 0 | 10 | 0.0% | ||
| <!-- | Total: | 118 | 134 | 88.06% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| service/history/shard/context.go | 1 | 78.13% | ||
| common/task/weighted_round_robin_task_scheduler.go | 2 | 88.56% | ||
| common/task/parallel_task_processor.go | 2 | 93.06% | ||
| common/persistence/metered.go | 2 | 80.87% | ||
| service/history/queue/timer_queue_processor_base.go | 3 | 77.87% | ||
| service/history/execution/mutable_state_builder.go | 3 | 78.26% | ||
| service/history/task/transfer_standby_task_executor.go | 4 | 87.35% | ||
| service/history/handler/handler.go | 4 | 96.43% | ||
| common/task/fifo_task_scheduler.go | 4 | 83.51% | ||
| service/frontend/api/handler.go | 4 | 75.68% | ||
| <!-- | Total: | 317 | --> |
| Totals | |
|---|---|
| Change from base Build 01903cd7-c1ac-49f3-a7a4-fe9da6c16ce7: | -0.1% |
| Covered Lines: | 104302 |
| Relevant Lines: | 146057 |
coveralls
commented
5 days ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| common/persistence/persistence-tests/persistenceTestBase.go | 0 | 2 | 0.0% | ||
| service/history/handler/handler.go | 0 | 2 | 0.0% | ||
| service/history/task/transfer_active_task_executor.go | 36 | 38 | 94.74% | ||
| common/persistence/persistence-tests/shardPersistenceTest.go | 0 | 10 | 0.0% | ||
| <!-- | Total: | 118 | 134 | 88.06% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| service/history/shard/context.go | 1 | 78.93% | ||
| common/task/weighted_round_robin_task_scheduler.go | 2 | 89.05% | ||
| common/task/fifo_task_scheduler.go | 2 | 87.63% | ||
| common/persistence/metered.go | 2 | 80.87% | ||
| service/matching/tasklist/matcher.go | 2 | 90.18% | ||
| service/matching/tasklist/task_reader.go | 2 | 77.72% | ||
| service/history/execution/mutable_state_builder.go | 3 | 78.26% | ||
| common/persistence/statsComputer.go | 3 | 98.18% | ||
| service/history/task/transfer_standby_task_executor.go | 4 | 87.35% | ||
| common/archiver/filestore/historyArchiver.go | 4 | 80.95% | ||
| <!-- | Total: | 327 | --> |
| Totals | |
|---|---|
| Change from base Build 01903cd7-c1ac-49f3-a7a4-fe9da6c16ce7: | -0.1% |
| Covered Lines: | 104314 |
| Relevant Lines: | 146061 |
coveralls
commented
4 days ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| common/persistence/persistence-tests/persistenceTestBase.go | 0 | 2 | 0.0% | ||
| service/history/handler/handler.go | 0 | 2 | 0.0% | ||
| service/history/task/transfer_active_task_executor.go | 36 | 38 | 94.74% | ||
| common/persistence/persistence-tests/shardPersistenceTest.go | 0 | 10 | 0.0% | ||
| <!-- | Total: | 118 | 134 | 88.06% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| service/history/shard/context.go | 1 | 78.13% | ||
| common/task/fifo_task_scheduler.go | 2 | 84.54% | ||
| common/persistence/metered.go | 2 | 80.87% | ||
| service/matching/tasklist/matcher.go | 2 | 90.91% | ||
| service/matching/tasklist/task_reader.go | 2 | 77.72% | ||
| service/history/task/task.go | 3 | 84.81% | ||
| service/history/execution/mutable_state_builder.go | 3 | 78.39% | ||
| common/persistence/statsComputer.go | 3 | 98.18% | ||
| service/history/handler/handler.go | 4 | 96.43% | ||
| service/history/queue/timer_queue_processor_base.go | 4 | 77.66% | ||
| <!-- | Total: | 340 | --> |
| Totals | |
|---|---|
| Change from base Build 01903cd7-c1ac-49f3-a7a4-fe9da6c16ce7: | -0.1% |
| Covered Lines: | 104279 |
| Relevant Lines: | 146061 |
coveralls
commented
4 days ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| common/persistence/persistence-tests/persistenceTestBase.go | 0 | 2 | 0.0% | ||
| service/history/handler/handler.go | 0 | 2 | 0.0% | ||
| service/history/task/transfer_active_task_executor.go | 36 | 38 | 94.74% | ||
| common/persistence/persistence-tests/shardPersistenceTest.go | 0 | 10 | 0.0% | ||
| <!-- | Total: | 118 | 134 | 88.06% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| service/history/shard/context.go | 1 | 78.13% | ||
| service/history/task/transfer_standby_task_executor.go | 2 | 87.04% | ||
| common/task/weighted_round_robin_task_scheduler.go | 2 | 89.05% | ||
| service/matching/tasklist/task_list_manager.go | 2 | 76.65% | ||
| common/persistence/sql/sqlplugin/mysql/task.go | 2 | 73.68% | ||
| common/persistence/metered.go | 2 | 80.87% | ||
| common/membership/hashring.go | 2 | 84.69% | ||
| service/matching/tasklist/matcher.go | 2 | 90.91% | ||
| service/matching/tasklist/task_reader.go | 2 | 77.72% | ||
| common/persistence/sql/sqlplugin/mysql/db.go | 2 | 79.49% | ||
| <!-- | Total: | 314 | --> |
| Totals | |
|---|---|
| Change from base Build 019056dc-98d1-4fa6-b475-a7aef51f4b90: | -0.1% |
| Covered Lines: | 104692 |
| Relevant Lines: | 146539 |
What changed? This *mostly** removes the cross-cluster feature.
Background
The Cross-cluster feature was the ability to launch and interact with child workflows in another domain. It included the ability to start child workflows and signal them. The feature allowed child workflows to be launched in the target domain even if it was active in another region.
Problems
The feature itself was something that very very few of our customers apparently needed, with very few customers interested in the problem of launching child workflows in another cluster, and zero who weren’t able to simply use an activity to make an RPC call to the other domain as one would with any normal workflow. The feature-itself was quite resource intensive: It was pull-based; spinning up a polling stack which polled the other cluster for work, similar to the replication stack. This polling behaviour made the latency characteristics fairly unpredictable and used considerable DB resources, to the point that we just turned it off. The Uber/Cadence team resolved that were there sufficient demand for the feature in the future, a push based mechanism would probably be significantly preferable. The feature itself added a nontrivial amount of complexity to the codebase in a few areas such as task processing and domain error handling which introduced difficult to understand bugs such as the child workflow dropping error https://github.com/uber/cadence/pull/5919
Decision to deprecate and alternatives
As of releases June 2024, the feature will be removed. The Cadence team is not aware of any users of the feature outside Uber (as it was broken until mid 2021 anyway), but as an FYI, it will cease to be available.
If this behaviour is desirable, an easy workaround is as previously mentioned: Use an activity to launch or signal the workflows in the other domain and block as needed.
PR details
This is a fairly high-risk refactor so it'll take some time to land. Broadly it:
Notable callouts
Testing
This is a pretty high risk change and the bar for testing should be fairly high, so I'll update the manual testing in this table as it's done:
there's obviously a bunch more possibilities with continue-as-new here too, but at a certain point I'm giong to have to rely on automation. There's been extremely little changes to the integration tests