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

Upgrade agent-js #2446

Merged
merged 1 commit into from
Apr 26, 2024
Merged

Upgrade agent-js #2446

merged 1 commit into from
Apr 26, 2024

Conversation

frederikrothenberger
Copy link
Contributor

@frederikrothenberger frederikrothenberger commented Apr 26, 2024

This PR upgrades agent-js to the latest version because it includes an improvement to the errors thrown when failing to decode cbor.


🟡 Some screens were changed

Copy link
Collaborator

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

Thanks!

This PR upgrades `agent-js` to the latest version because it
includes an improvement to the errors thrown when failing to
decode cbor.
@peterpeterparker
Copy link
Member

If interesting, in ic-js I did not upgraded yet agent-js because an issue was reported:

dfinity/ic-js#593 (comment)

I asked @krpeacock if the related patch would be merged soon but, it seems to not have been patched yet so, I'm still waiting.

Don't know though if the issue is a blocker or a race condition in some particular use case.

@frederikrothenberger
Copy link
Contributor Author

@peterpeterparker: Thanks for your input. I'll merge this and then set the retry count to 10 in the agent, which seems to fix this (albeit in an inelegant way).

@frederikrothenberger frederikrothenberger added this pull request to the merge queue Apr 26, 2024
Merged via the queue into main with commit 391f930 Apr 26, 2024
66 checks passed
@frederikrothenberger frederikrothenberger deleted the frederik/bump-agent-js branch April 26, 2024 08:17
frederikrothenberger pushed a commit that referenced this pull request Apr 26, 2024
This PR configures the number of retries to 10 (as opposed to just
3, which is the default). This makes the agent more resilient
against watermark check failures, because the likelihood of hitting
only replicas that are behind gets lowered a lot and retrying takes time
too.

Addresses [this comment](#2446 (comment)).
github-merge-queue bot pushed a commit that referenced this pull request Apr 26, 2024
Configure up to 10 retries on agnet failure

This PR configures the number of retries to 10 (as opposed to just
3, which is the default). This makes the agent more resilient
against watermark check failures, because the likelihood of hitting
only replicas that are behind gets lowered a lot and retrying takes time
too.

Addresses [this comment](#2446 (comment)).
@krpeacock
Copy link
Contributor

Yeah, it's taken me longer than I intended, and I still don't have it working against prod. Having the conference come up didn't help either

peterpeterparker added a commit to dfinity/ic-js that referenced this pull request Apr 29, 2024
# Motivation

The migration to  pure JS BLS verification in agent-js v1.2.0 has a notable impact on bundle size (roughly -15% according [forum post](https://forum.dfinity.org/t/agent-js-1-2-0-is-released/28881/2?u=peterparker)). Therefore it makes sense to promote those changes.

# Notes

To prevent the issue reported on the [forum](https://forum.dfinity.org/t/timestamp-failed-to-pass-the-watermark-after-retrying-the-configured-3-times/29180/3?u=peterparker) to happen, the number of retry for the agent has been set to 10 similiar has what was done in II (see [PR](dfinity/internet-identity#2446))

# Changes

- `npm run update:agent`
- set number of retry to 10 and expose a parameter to tweak the value if required
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