Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Use parse_duration for fed client timeout/retry options#15778

Closed
MatMaul wants to merge 2 commits intodevelopfrom
mv/parse-duration
Closed

Use parse_duration for fed client timeout/retry options#15778
MatMaul wants to merge 2 commits intodevelopfrom
mv/parse-duration

Conversation

@MatMaul
Copy link
Contributor

@MatMaul MatMaul commented Jun 13, 2023

Pull Request Checklist

@MatMaul MatMaul changed the title Use parse_duration for newly introduced options Use parse_duration for fed client timeout/retry options Jun 13, 2023
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Sorry for drive-by while in draft. Do docs need updating?

self._store = hs.get_datastores().main
self.version_string_bytes = hs.version_string.encode("ascii")
self.default_timeout = hs.config.federation.client_timeout
self.default_timeout = hs.config.federation.client_timeout / 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

This is in seconds now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parse_duration returns milliseconds.

else:
_sec_timeout = self.default_timeout
if timeout is None:
timeout = int(self.default_timeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is also in seconds now...

Copy link
Contributor

Choose a reason for hiding this comment

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

For bonus points, leave a comment saying this is in seconds/milliseconds/whatever.

For a gold star, rename to self.timeout_seconds etc!

if timeout is None:
timeout = int(self.default_timeout)

_sec_timeout = timeout / 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

which means that this is in kiloseconds if timeout is None, but seconds otherwise. That seems wrong.

Ditto below.

@MatMaul
Copy link
Contributor Author

MatMaul commented Jun 14, 2023

@DMRobertson docs do need some update, seems like I did the changes and then didn't commit them...

@MatMaul
Copy link
Contributor Author

MatMaul commented Jun 15, 2023

Superseeded by #15783.

@MatMaul MatMaul closed this Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants