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

Make the polling interval configurable. #170

Merged
merged 6 commits into from
Dec 5, 2024
Merged

Conversation

manoranjith
Copy link
Contributor

@manoranjith manoranjith commented Dec 4, 2024

  • feat: make coprocessor sync interval configurable

    • Currently, this value is hardcoded to 10s. This could be too long,
      leading to artificial delay in the ciphertext being available in the
      coprocessor db.

    • Now, make it configurable via environment variable.

  • feat: make worker job polling interval config

    • Previously, the value was hardcoded to 5000ms. This could lead to long
      wait between processing jobs and hence a delay in result being
      available in the coprocessor db.

    • Now, make this interval configurable via command line argument. Can be
      fine tuned as per needed performance.

  • chore: bump version to v0.1.2

Closes #169

@manoranjith manoranjith force-pushed the mano/configurable-polling branch from 7ff32f3 to 9b76b7c Compare December 4, 2024 10:57
@manoranjith manoranjith force-pushed the mano/configurable-polling branch from 9b76b7c to 80390b1 Compare December 4, 2024 17:28
@manoranjith manoranjith marked this pull request as ready for review December 4, 2024 17:37
@manoranjith manoranjith force-pushed the mano/configurable-polling branch from 225fe2f to ea0f02b Compare December 4, 2024 17:44
@manoranjith manoranjith requested a review from fd0r December 4, 2024 18:01
@fd0r
Copy link
Contributor

fd0r commented Dec 4, 2024

Don't forget to follow conventional commit when adding this new config 😉 (aka breaking change)

https://www.conventionalcommits.org/en/v1.0.0/

@manoranjith
Copy link
Contributor Author

The link is fixed.

@manoranjith
Copy link
Contributor Author

manoranjith commented Dec 5, 2024

@fd0r Thanks for highlighting this could be a breaking change.

I guess, i can make this a more soft change by adding a default value for FHEVM_COPROCESSOR_SYNC interval as well, when it is not set.

The other config already has a default.

Both values fallback to default, if not configured. I guess, this makes it not a breaking change ???

- Previously, the value was hardcoded to 5000ms. This could lead to long
  wait between processing jobs and hence a delay in result being
  available in the coprocessor db.

- Now, make this interval configurable via command line argument. Can be
  fine tuned as per needed performance.
- Currently, this value is hardcoded to 10s. This could be too long,
  leading to artificial delay in the ciphertext being available in the
  coprocessor db.

- Now, make it configurable via environment variable.
@manoranjith manoranjith force-pushed the mano/configurable-polling branch from de9d400 to 59ef973 Compare December 5, 2024 14:26
Copy link
Collaborator

@dartdart26 dartdart26 left a comment

Choose a reason for hiding this comment

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

Looks good! I just asked @goshawk-3 to check the select! statement as we discussed it, but maybe he got deeper on that.

This versioning scheme is more aligned with fhevm versioning scheme.
Copy link
Contributor

@fd0r fd0r left a comment

Choose a reason for hiding this comment

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

Ran the integration tests of fhevm-devops with it and it worked

@manoranjith manoranjith merged commit 70bb15a into main Dec 5, 2024
5 checks passed
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.

Investigate delay in ciphertext being available after computation
4 participants