IWF-473: Upgrade Cadence dependencies #548
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #548 +/- ##
=======================================
Coverage 66.06% 66.06%
=======================================
Files 62 62
Lines 6927 6927
=======================================
Hits 4576 4576
Misses 2072 2072
Partials 279 279 ☔ View full report in Codecov by Sentry. |
| @@ -55,12 +50,6 @@ func (h *handler) ApiV1WorkflowStateStart(c *gin.Context, t *testing.T) { | |||
| } | |||
|
|
|||
| if req.GetWorkflowStateId() == State1 { | |||
| nowInt, err := strconv.Atoi(req.StateInput.GetData()) | |||
There was a problem hiding this comment.
This was not being used in the test assertions and it was causing this test to be flaky for some weird reason, so I removed it
There was a problem hiding this comment.
i think that's for the best, if the value isn't being used but is causing issues it should be removed 👍
There was a problem hiding this comment.
Do you have a link to a failed pipeline where this test was flaky? What was the error exactly?
There was a problem hiding this comment.
Yes, you can look at the actions in the previous commit in this PR: 8194a71
I believe this was flaky because while the Cadence tests failed(https://github.com/indeedeng/iwf/actions/runs/13464338908/job/37627766325), the All Tests job passed (https://github.com/indeedeng/iwf/actions/runs/13464338906/job/37626732846) and the latter runs the same Cadence tests.
The error was this: https://github.com/indeedeng/iwf/actions/runs/13464338908/job/37627766325#step:4:5143
errors.go:6: TestIgnoreAlreadyStartedWorkflowCadence - Test failed with error: strconv.Atoi: parsing "": invalid syntax
There was a problem hiding this comment.
When I looked closer, it seemed that the value wasn't being passed to the test router. And it the property it wrote to in the router wasn't used in the test for any assertion, so I removed this bit. The tests assertions haven't changed i.e. it's doing the same checks as before
samuel27m
requested review from
lwolczynski,
ktrops,
stevo89519,
N-give and
mixydubs
| networks: | ||
| - testing-network | ||
| zookeeper: |
There was a problem hiding this comment.
Looks like zookeeper is no longer a dependency with the latest version of Cadence, is that right?
There was a problem hiding this comment.
Yes, this is the cadence MR that removed Zookeeper for reference: cadence-workflow/cadence@4cb4443
And this is the file I used to look at how Cadence has their Docker setup: https://github.com/cadence-workflow/cadence/blob/master/docker/docker-compose-es-v7.yml
| KAFKA_ADVERTISED_LISTENERS: PLAINTEXT://kafka:9092 | ||
| KAFKA_LISTENERS: PLAINTEXT://0.0.0.0:9092 | ||
| KAFKA_ZOOKEEPER_CONNECT: zookeeper:2181 | ||
| # KRaft settings |
There was a problem hiding this comment.
Answered below: #548 (comment)
| KAFKA_ADVERTISED_LISTENERS: PLAINTEXT://kafka:9092 | ||
| KAFKA_LISTENERS: PLAINTEXT://0.0.0.0:9092 | ||
| KAFKA_ZOOKEEPER_CONNECT: zookeeper:2181 | ||
| # KRaft settings |
There was a problem hiding this comment.
Same here. Not sure what KRaft is
There was a problem hiding this comment.
https://developer.confluent.io/learn/kraft/
Apache Kafka Raft (KRaft) is the consensus protocol that was introduced in KIP-500 to remove Apache Kafka’s dependency on ZooKeeper for metadata management. This greatly simplifies Kafka’s architecture by consolidating responsibility for metadata into Kafka itself, rather than splitting it between two different systems: ZooKeeper and Kafka. KRaft mode makes use of a new quorum controller service in Kafka which replaces the previous controller and makes use of an event-based variant of the Raft consensus protocol.
There was a problem hiding this comment.
Basically seems to be the way forward so that we no longer need Zookeeper. But mainly, I copied most of the configs from the Cadence repo, so we're doing the same configs as they recommend right now
Description
Checklist
Related Issue
Closes #432