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

Awakeable identifiers following the new ID scheme #238

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Conversation

AhmedSoliman
Copy link
Collaborator

@AhmedSoliman AhmedSoliman commented Jan 25, 2024

Awakeable identifiers following the new ID scheme

Bonus: s/assumpsions/assumptions/

Server side in restatedev/restate#1120

Test Plan:

Using sample code to generate an awakeable and resolved it with

curl localhost:8080/dev.restate.Awakeables/Resolve -H 'content-type: application/json' -d '{"id": "prom_1NMyOAvDK2CcBjUH4Rmb7eGBp0DNNDnmsAAAAAQ", "json_result": ""}'
[restate] [2024-01-25T18:54:01.682Z] DEBUG: [CheckoutProcess/checkout] [inv_13q6r7rhzaG85fs0bfyqW8cubIUYJFjvBT] : Resuming (replaying) function.
Awakeable is:  prom_1NMyOAvDK2CcBjUH4Rmb7eGBp0DNNDnmsAAAAAQ
Awakeable COMPLETED  prom_1NMyOAvDK2CcBjUH4Rmb7eGBp0DNNDnmsAAAAAQ

Copy link

github-actions bot commented Jan 25, 2024

Test Results

102 files  ±0  102 suites  ±0   15m 49s ⏱️ + 5m 37s
 93 tests ±0   86 ✅  -  7  0 💤 ±0   7 ❌ + 7 
232 runs  ±0  214 ✅  - 18  0 💤 ±0  18 ❌ +18 

For more details on these failures, see this check.

Results for commit 90fa26b. ± Comparison against base commit 766a642.

♻️ This comment has been updated with latest results.

@AhmedSoliman AhmedSoliman marked this pull request as ready for review January 26, 2024 09:22
Bonus: `s/assumpsions/assumptions/`

Test Plan:

Using sample code to generate an awakeable and resolved it with
```
curl localhost:8080/dev.restate.Awakeables/Resolve -H 'content-type: application/json' -d '{"id": "prom_1NMyOAvDK2CcBjUH4Rmb7eGBp0DNNDnmsAAAAAQ", "json_result": ""}'
```

```
[restate] [2024-01-25T18:54:01.682Z] DEBUG: [CheckoutProcess/checkout] [inv_13q6r7rhzaG85fs0bfyqW8cubIUYJFjvBT] : Resuming (replaying) function.
Awakeable is:  prom_1NMyOAvDK2CcBjUH4Rmb7eGBp0DNNDnmsAAAAAQ
Awakeable COMPLETED  prom_1NMyOAvDK2CcBjUH4Rmb7eGBp0DNNDnmsAAAAAQ
```
Copy link
Contributor

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

Once it's green you can merge.

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Should we bump the protocol version to mark this change as an non-backwards compatible change?

@slinkydeveloper
Copy link
Contributor

Should we bump the protocol version to mark this change as an non-backwards compatible change?

There's no version to bump yet, still need to solve #235 first

@AhmedSoliman AhmedSoliman merged commit 5245539 into main Jan 26, 2024
2 of 5 checks passed
@AhmedSoliman
Copy link
Collaborator Author

For posterity, tests pass locally but we have a full mesh of dependency such that there is no way e2e tests will pass without merging the sdk first.

@AhmedSoliman AhmedSoliman deleted the pr238 branch January 26, 2024 14:02
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