-
Notifications
You must be signed in to change notification settings - Fork 1
Feature/sc 241416 allow max count tuning #71
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
base: feature/sc-241273-add-interpolation-to-Tranpose-n-Sync
Are you sure you want to change the base?
Feature/sc 241416 allow max count tuning #71
Conversation
… into feature/sc-241416-allow-maxCount-tuning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes work fine, in particular for Attribute search with retrieve values and for Asset values downloader recipe behave as expected.
It also works well for the PIWebAPI Toolbox (assuming we don't want to use it is a batch size param here, just get the first max_count
records back - it is a connector used only for experimentation, so I assume that is right).
BUT for "Event frames search" and "Event frames retriever", while changing the parameter values does change the value in the API call, it would probably be better not to have that param exposed in the UI. Since:
- it only affects the number of values coming back in each response within each returned item, where each item represents an attribute relating to the event frame. And then only for the recorded data mode. Normally there are relatively few of these value subitems for each attribute, and the default of max_count 10000 is very unlikely to be exceeded
- as such it does not really work as a batch size tuning parameter for the requests as a whole so is not useful - it will not be obvious what is does anyway
- AND if max_count is below the number of values for a given attribute in an event frame (you can test that by setting it very low), the rest of the values are NOT returned anyway. In essence the pagination around this does not work (due to the endpoint behaving unlike the others), and exposing this param would result in a bug if anyone sets the value to something low
- for the other modes - e.g. interpolated mode - it does not do anything at all (the documentation doesn't talk about it, it is ignored in the API), so will just be confusing
So I think we should consider not having that param for the Event frames related components.
👍 maxCount selector made invisible for EF tools in 3d5ef56 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now in terms of code 👍
We need to be careful how we explain this change I think.
I don't see any change log changes here? Or for the other PRs. I think I need to check all these too, and version changes.
Is there or will there be a PR with that stuff I can take a look at?
I pushed all the log changes and version bumps in release/1.3.0, I just created the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Story details: https://app.shortcut.com/dataiku/story/241416