fix: requestBuffer #49
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NorbertNader
requested review from
fpauer,
boweihan,
diehbria and
gareth-amazon
NorbertNader
commented
Feb 7, 2022
| @@ -177,32 +190,41 @@ export class IotAppKitDataModule implements DataModule { | |||
|
|
|||
| const updatedSubscription = Object.assign({}, subscription, subscriptionUpdate) as Subscription; | |||
|
|
|||
| const request = this.getAdjustedRequest(updatedSubscription.request); | |||
|
|
|||
| const viewport = { | |||
There was a problem hiding this comment.
This part is important because we want the actual "visible" viewport not the request viewport that might be buffered. E.g. in the iot-connector component we listen for dateRangeChange which invokes the update method with the chart viewport, not the requested viewport. This means that if we are looking at a chart for a 10 minute range but actually request 30 minutes of data, we will still request raw data, although the resolution mapping is set to request '1m' resolution above 15 minute viewport range.
There was a problem hiding this comment.
Makes a lot of sense.
NorbertNader
commented
Feb 7, 2022
| dataSource.initiateRequest( | ||
| { | ||
| onError, | ||
| onSuccess, | ||
| query, | ||
| request: { | ||
| viewport: { | ||
| duration: FIFTY_FIVE_MINUTES, | ||
| duration: HOUR_IN_MS * 65, |
There was a problem hiding this comment.
This test makes sure we determine the resolution to request based on the visible viewport, not the requested viewport, which might be buffered.
NorbertNader
force-pushed
the
fix-request-buffer
branch
from
4e62bd8
to
35c7cc4
Compare
NorbertNader
commented
Feb 8, 2022
| }) | ||
| }); | ||
|
|
||
| this.registerRequest({ queries, request: { ...request, viewport: { start: reqStart, end: reqEnd } }, viewport }); |
There was a problem hiding this comment.
We register a request only for time ranges of uncached data. Covered by tests.
* remove legacy sitewise resource * remove requestInformations from initiateRequest signature * fix requests to only be made for uncached data
NorbertNader
force-pushed
the
fix-request-buffer
branch
from
35c7cc4
to
c89bc6e
Compare
diehbria
requested changes
Feb 9, 2022
NorbertNader
commented
Feb 10, 2022
| }); | ||
| }); | ||
|
|
||
| it('only fetches uncached data for multiple properties', async () => { |
There was a problem hiding this comment.
This is a full e2e test for the changes made in this CR. Basically making sure we do not over request.
diehbria
reviewed
Feb 10, 2022
packages/core/src/data-module/subscription-store/subscriptionStore.spec.ts
Outdated
Show resolved
Hide resolved
boweihan
reviewed
Feb 11, 2022
| queries, | ||
| start, | ||
| end, | ||
| viewport, |
There was a problem hiding this comment.
how about calling this bufferedViewport for clarity?
There was a problem hiding this comment.
I want to avoid that because the viewport might not actually be buffered.
There was a problem hiding this comment.
hmm potentiallyBufferedViewport? Just kidding, makes sense :)
NorbertNader
force-pushed
the
fix-request-buffer
branch
from
28b2901
to
51bb27b
Compare
diehbria
approved these changes
Feb 11, 2022
boweihan
reviewed
Feb 11, 2022
| @@ -130,19 +128,15 @@ export const createDataSource = (siteWise: IoTSiteWiseClient): DataSource<SiteWi | |||
| const client = new SiteWiseClient(siteWise); | |||
| return { | |||
| name: SITEWISE_DATA_SOURCE, | |||
| initiateRequest: ({ query, request, onSuccess, onError }) => { | |||
| initiateRequest: ({ query, request, onSuccess, onError }, requestInformations) => { | |||
There was a problem hiding this comment.
yup, finally using it as intended :)
boweihan
reviewed
Feb 11, 2022
sheilaXu
pushed a commit
that referenced
this pull request
Sep 23, 2022
See all commit messages below Fixes the nested camera view locations for activeCameraSettings (#102) * Fixes the nested camera view locations for activeCameraSettings The problem is that the node.transform is not in world coordinates. To accurately represent this we create the transform from the actualy object3D world position, rotation, and scale * Clean up based on feedback * Rebase needed Fix target calculation for Camera settings (#104) Have the Camera Preview pull from the selected node rather than using active camera settings (#101) Adjusting the active Camera settings also affected the main camera's settings and was a bug. Utilizing the camera component on the selected node allowed for the same render without the issue as the active Camera settings aren't necessarily set to active by editing anyway Update the camera component inspector editor to match the UX (#99) Visual restructuring Bug fixes Code clean up Adds a CameraPreview component (#97) This performs a LateRender to a smaller viewport designated by the tracked div element When a camera is selected the div and the render of the camera's view will appear when in edit mode Adds the activeCamera property (#66) Based on this property we set the active camera unless we have a selected data binding as that should take precedence Make TopBar always visible and Add Camera list (#64) Clicking a camera name in the list will set that camera as the active camera in either editing or viewing Add an incremental count to the Camera object to give unique names since creation Revert "Add activeCamera Property to Scene Viewer (#56)" (#61) This reverts commit 98bce138a4bf572b6f25562c458ace8d7ac3f560. Add activeCamera Property to Scene Viewer (#56) State setup seems to appear incorrectly in storybook such that the Camera View is not utlizing the activeCamera unless the store has already been created. This is something that will need to be looked into further. Add Set from current view, button to inspector (#52) Also refactors how we set the object from the current view Fixes the bug where we only support one parent for rotational transform Add basic Inspector Editor for the Camera Component (#49) Change Camera creation mode to View Camera (From current) as in design (#46) Update the CameraComponent to be creatable and selectable (#45) This introduces the ability to set a globally stored active camera which can alter the settings and viewing location of the main camera. These are less cameras themselves than editor and data representations
TheEvilDev
pushed a commit
that referenced
this pull request
Nov 2, 2022
* Fix requestBuffer * Remove legacy sitewise data source * Remove fetchAgggreagtedData boolean from settings * Fix requests to only be made for uncached data * Fix gesture tests Co-authored-by: Norbert Nader <nnader@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Tests
updated unit tests and integration tests
Legal
This project is available under the Apache 2.0 License.