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 subscription data calls more efficient #1538

Merged
merged 10 commits into from
Dec 11, 2023

Conversation

matiascognite
Copy link
Contributor

@matiascognite matiascognite commented Dec 4, 2023

Description

Use a more efficient method to query the subscription list data API

The timeout parameter will enable active wait:
Until there is more data available, we will keep the request waiting in the backend. Only when the timeout has expired, we will return.

Not a visible change to the user; no need to update version or changelog?

Checklist:

  • Tests added/updated.
  • Documentation updated. Documentation is generated from docstrings - these must be updated according to your change.
    If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.
  • Changelog updated in CHANGELOG.md.
  • Version bumped. If triggering a new release is desired, bump the version number in _version.py and pyproject.toml per semantic versioning.

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Merging #1538 (cc8c35e) into master (58a389c) will increase coverage by 0.03%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1538      +/-   ##
==========================================
+ Coverage   91.78%   91.82%   +0.03%     
==========================================
  Files         120      120              
  Lines       15623    15623              
==========================================
+ Hits        14340    14346       +6     
+ Misses       1283     1277       -6     
Files Coverage Δ
cognite/client/_api/datapoints_subscriptions.py 100.00% <ø> (ø)
cognite/client/_version.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@matiascognite
Copy link
Contributor Author

Thanks for review, even if I didn't ask for it @haakonvt !
Now that we actually implemented it in the backend, the tests pass, but I have some questions

  • Given that we change the method signature, we should probably do a (minor) version bump?
  • Is poll_timeout a good name? We have pollTimeoutSeconds in the API, but I have the impression that python defaults to seconds (correct me if I'm wrong)
  • The behavior changes. If I read it correctly, it will now block while waiting for new data. Before it would just respond with an empty change immediately. It should mostly be an improvement though, and also protect against writing very heavy applications with constant polling, by accident.

@haakonvt
Copy link
Contributor

haakonvt commented Dec 7, 2023

@matiascognite

  • Given that we changed the method signature, we should probably do a (minor) version bump?

Yup, I agree!

  • Is poll_timeout a good name? We have pollTimeoutSeconds in the API, but I have the impression that python defaults to seconds (correct me if I'm wrong)

Sounds great 👌 Not sure what you mean with "python defaults to seconds" though? Your docstring description is very clear.

  • The behavior changes. If I read it correctly, it will now block while waiting for new data. Before it would just respond with an empty change immediately. It should mostly be an improvement though, and also protect against writing very heavy applications with constant polling, by accident.

Yeah, for sure an improvement! Particularly for our pyodide users who might not know time.sleep doesn't work and thus unknowingly are sending requests in a "DDOS pattern"... Anyway, the datapoints subscriptions API has been issuing warnings that the SDK implementation is still in alpha, so we are free to change it! (and even if it wasn't in alpha, I would merge this feature without hesitation)

@matiascognite
Copy link
Contributor Author

Sounds great 👌 Not sure what you mean with "python defaults to seconds" though? Your docstring description is very clear.

In java and other languages, timestamps are often measured in milliseconds. So a variable named timeout could be thought to be given in milliseconds, unless it is named timeoutSeconds. Witch is also the name in the API itself.

However, in python, I have the impression that time is measured in seconds, unless specified otherwise.

Anyway, if you are happy, I am happy :)

@matiascognite matiascognite marked this pull request as ready for review December 7, 2023 14:18
@matiascognite matiascognite requested review from a team as code owners December 7, 2023 14:18
Copy link
Contributor

@haakonvt haakonvt left a comment

Choose a reason for hiding this comment

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

In java and other languages, timestamps are often measured in milliseconds. So a variable named timeout could be thought to be given in milliseconds, unless it is named timeoutSeconds. Witch is also the name in the API itself.
However, in python, I have the impression that time is measured in seconds, unless specified otherwise.
Anyway, if you are happy, I am happy :)

Today I learned! At least epoch time is given in float sec (time.time()). Looks very good to me!

@matiascognite matiascognite enabled auto-merge (squash) December 11, 2023 12:05
@matiascognite matiascognite merged commit 87b3ac0 into master Dec 11, 2023
7 checks passed
@matiascognite matiascognite deleted the addTimeoutToSubscriptions branch December 11, 2023 12:09
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.

2 participants