-
Notifications
You must be signed in to change notification settings - Fork 113
Stat fetch default #3963
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: main
Are you sure you want to change the base?
Stat fetch default #3963
Conversation
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
…ak out implementation helpers, fetch default behavior tweaks Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3963 +/- ##
==========================================
+ Coverage 94.24% 94.27% +0.02%
==========================================
Files 129 130 +1
Lines 16169 16372 +203
Branches 3723 3815 +92
==========================================
+ Hits 15239 15435 +196
- Misses 928 935 +7
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
…hing Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
| const memberName = segments[2]; | ||
| const parentPath = segments.slice(0, 2).join("/"); | ||
| const parentUri = uri.with({ path: `/${parentPath}` }); | ||
| const parentKey = "list" + this.getQueryKey(uri) + "_" + parentUri.toString(); |
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.
With the fetch by default is enabled, is there still a missed opportunity to share the request when two different callers issue stat (and other calls) for the same resource - one is with fetch=true while the other does not provider the fetch parameter?
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.
As noted in the PR description, the behavior for fetch=true remains unchanged. The fetchByDefault logic is an optimization of the default behavior (fetch=false); therefore, to take advantage of this new lookup logic, the fetch=true query parameter must be removed from extender calls.
Due to this, the request reuse case would be when fetch=false and no fetch parameter is present, which is currently implemented.
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 scenario is
- fetchByDefault enabled
- cache is empty
two requests are issued by (possibly different extenders):
- fetch=true (because the caller 1 wants up-to-date results)
- without fetch parameter (caller 2 just wants to know the info and does not require the most up-to-date version)
The result will be two concurrent calls to the backend for the same information.
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.
I see
the current implementation does not have the granularity to separate the remoteLookup and lookup call since they are both within the wrapped promise. It would be a large refactor to separate the calls such that the fetch=false fallback remoteLookup call and fetch=true remoteLookup call would be reused.
Signed-off-by: Jace Roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
…d infinite loop Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
packages/zowe-explorer-api/__tests__/__unit__/fs/BaseProvider.unit.test.ts
Show resolved
Hide resolved
packages/zowe-explorer/__tests__/__unit__/trees/dataset/DatasetFSProvider.unit.test.ts
Outdated
Show resolved
Hide resolved
packages/zowe-explorer/__tests__/__unit__/trees/dataset/DatasetFSProvider.unit.test.ts
Outdated
Show resolved
Hide resolved
packages/zowe-explorer/__tests__/__unit__/utils/AuthUtils.unit.test.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
t1m0thyj
left a comment
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.
Implementation LGTM, thanks @jace-roell! Will approve once debug statements are removed.
zFernand0
left a comment
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.
Changes MSTM! 😋
Left a small comment asking for some tsdoc, but everything LGTM! 🙏
Will do some testing before I approve 🥳
Signed-off-by: jace-roell <[email protected]>
Proposed changes
Modifies the behavior of FileSystem calls in regards to fetching and caching values, aswell as caching similar in-flight requests to avoid redundant calls.
Feature Flags
Currently is used for the fetchByDefault feature flag, this flag will be removed prior to merging of this PR, but it is kept in order to allow for toggling of behavior when testing
fetchByDefault true vs false
fetch=false (default): will only look in the fileSystem cache (this.root) for entries
fetch=true: will do a remote lookup, regardless of a value in the cache
fetch=false (default): will do a lookup in the fileSystem cache but will fallback to a remote lookup in the case where one is not found
fetch=true: will ignore values in the FileSystem cache, does a remote lookup regardless
How to toggle a feature flag:
Developer: Open Extensions FolderfetchByDefaultfeature flag may be modified in thefeature-flags.jsonfileRequest caching
Enhanced Dataset Caching
Catching invalid DS URIs
stat()calls with URIs that could not exist as data setsTrailing slash fix
Example Script:
Release Notes
Milestone:
Changelog:
Types of changes
Checklist
General
yarn workspace vscode-extension-for-zowe vscode:prepublishpnpm --filter vscode-extension-for-zowe vscode:prepublishCode coverage
Deployment
Further comments