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.

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

Remove Fixed Dapr Version Constraint #147

Merged
merged 16 commits into from
Feb 19, 2025

Conversation

ruokun-niu
Copy link
Contributor

Description

We previously had to use a fixed verison of the dapr runtime (1.10.0), as we would run into the following error when we try to run drasi init with the latest version of dapr:

error saving actor transaction state: using transactions with MongoDB requires connecting to a replica set

This also restricted us to use the 1.9.0 version of the dapr sidecar.

Initially, I thought this was an issue with how we setup mongo in the Drasi cluster. However, after exploring a bit, I realized that the issue was with how we set up mongo as the dapr statestore. When we specify the mongo during the state store definition, we need to specifically point towards a mongo replicaset, which can be defined in the connectionString field.

NOTE: In the official dapr documentation on using mongo as a state store, connectionString is not listed as a valid field. However, according to this discord discussion and this PR, this field is actually added in 2024 and they just forgot to update the doc site. By defining a connection string to the replicaset in infra.yaml, dapr will correctly use the replica set as the mongo actor state store, which allows us to install Drasi with the latest version of dapr without any trouble.

The description below is generated by copilot :)

This pull request includes changes to the cli package to update the configuration for Dapr versions and MongoDB connection settings. The most important changes are as follows:

Updates to Dapr version configuration:

  • cli/cmd/init.go: Updated the default values for dapr-runtime-version to be empty (which is the equivalent of latest) and dapr-sidecar-version to "latest".

Changes to MongoDB connection settings:

Type of change

  • This pull request fixes a bug in Drasi and has an approved issue (issue link required).
  • This pull request adds or changes features of Drasi and has an approved issue (issue link required).
  • This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Drasi (issue link optional).

Fixes: #issue_number

@ruokun-niu ruokun-niu marked this pull request as draft February 3, 2025 18:58
@ruokun-niu ruokun-niu marked this pull request as ready for review February 3, 2025 20:47
@@ -372,7 +372,7 @@ jobs:
permissions:
packages: write
contents: read
runs-on: e2e-tester
runs-on: ubuntu-latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget about this.

- name: password
value:
- name: connectionString
value: "mongodb://drasi-mongo-0.drasi-mongo:27017/?replicaSet=rs0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always going to connect to the -0 instance? Is there somekind of service proxy to route to any of the available instances?

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 think we can use the service name (drasi-mongo) instead. I'll test this to make sure things are working

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 have just tested this. We can use the service name in the connectionstring (`"mongodb://drasi-mongo:27017/?replicaSet=rs0")

Copy link
Contributor

@amansinghoriginal amansinghoriginal left a comment

Choose a reason for hiding this comment

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

DAPR version change looks good to me. But there seems to be some unrelated changes in this PR also. Is my understanding correct?

@@ -31,7 +31,7 @@
queryIds.Add(queryId);
}


Console.WriteLine("QueryContainerId: " + queryContainerId);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem related to DAPR version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The addition of await on line 142 in this file is minor fix for the result reaction. Encountered this occasionally when I attempted the smoke test

@ruokun-niu ruokun-niu merged commit 2227a1c into drasi-project:main Feb 19, 2025
32 checks passed
@ruokun-niu ruokun-niu deleted the dapr-investigation branch February 19, 2025 19:59
ruokun-niu added a commit to ruokun-niu/drasi-platform that referenced this pull request Mar 4, 2025
…/drasi-platform into review-debug-reaction

* 'review-debug-reaction' of https://github.com/ruokun-niu/drasi-platform:
  Transition to Native Multi-Platform Builds for Docker Images (drasi-project#161)
  Bump jinja2 from 3.1.4 to 3.1.5 in /reactions/sdk/python (drasi-project#135)
  Bump vite in /reactions/signalr/signalr-reaction/clients/vue (drasi-project#142)
  Bump nanoid in /reactions/signalr/signalr-reaction/clients/react/example (drasi-project#131)
  Bump openssl from 0.10.66 to 0.10.71 in /query-container (drasi-project#160)
  Bump golang.org/x/crypto from 0.26.0 to 0.31.0 in /cli (drasi-project#132)
  Bump nanoid in /reactions/signalr/signalr-reaction/clients/vue/example (drasi-project#130)
  Bump openssl from 0.10.68 to 0.10.71 in /sources/sdk/rust/example/proxy (drasi-project#158)
  Bump io.undertow:undertow-core from 2.3.15.Final to 2.3.17.Final in /sources/sdk/java (drasi-project#127)
  Bump esbuild from 0.16.17 to 0.25.0 in /dev-tools/vscode/drasi (drasi-project#151)
  Bump openssl from 0.10.66 to 0.10.70 in /control-planes/mgmt_api (drasi-project#148)
  Remove Fixed Dapr Version Constraint (drasi-project#147)
  Gremlin Reaction hotfix (drasi-project#149)
ruokun-niu added a commit to ruokun-niu/drasi-platform that referenced this pull request Mar 4, 2025
…si-platform into debug-base-images

* 'debug-base-images' of https://github.com/ruokun-niu/drasi-platform:
  Transition to Native Multi-Platform Builds for Docker Images (drasi-project#161)
  Bump jinja2 from 3.1.4 to 3.1.5 in /reactions/sdk/python (drasi-project#135)
  Bump vite in /reactions/signalr/signalr-reaction/clients/vue (drasi-project#142)
  Bump nanoid in /reactions/signalr/signalr-reaction/clients/react/example (drasi-project#131)
  Bump openssl from 0.10.66 to 0.10.71 in /query-container (drasi-project#160)
  Bump golang.org/x/crypto from 0.26.0 to 0.31.0 in /cli (drasi-project#132)
  Bump nanoid in /reactions/signalr/signalr-reaction/clients/vue/example (drasi-project#130)
  Bump openssl from 0.10.68 to 0.10.71 in /sources/sdk/rust/example/proxy (drasi-project#158)
  Bump io.undertow:undertow-core from 2.3.15.Final to 2.3.17.Final in /sources/sdk/java (drasi-project#127)
  Bump esbuild from 0.16.17 to 0.25.0 in /dev-tools/vscode/drasi (drasi-project#151)
  Bump openssl from 0.10.66 to 0.10.70 in /control-planes/mgmt_api (drasi-project#148)
  Remove Fixed Dapr Version Constraint (drasi-project#147)
  Gremlin Reaction hotfix (drasi-project#149)
  EventHub Source (drasi-project#152)
  Configured the EventGrid Reaction to use different schemas (drasi-project#138)
  AWS IRSA support and Eventbridge reaction (drasi-project#140)
  init logging (drasi-project#153)
  Dotnet source sdk (drasi-project#144)
  Refactor Relational Source and add MySQL Support. (drasi-project#146)
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.

4 participants