-
Notifications
You must be signed in to change notification settings - Fork 19
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
KETTLE-63: Added an recursive file datasource #37
base: main
Are you sure you want to change the base?
Conversation
lib/dataSource-recursiveFile.js
Outdated
} | ||
var i = 0; | ||
|
||
(function next() { |
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.
Nuts and deeply unidiomatic!
This can all be vastly simpler than as implemented. We don't require for this to be backed by a filesystem which will change after startup - it is only ever applied for test data for unit or acceptance tests. We can simply do one grand scan at startup, build a table of which directory which file is in, and then for ever afterwards just look up that file in the table.
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.
This would mean that we would need to index the entire filesystem on startup though, wouldn't it? Since we dont know which folder the request is gonna be for, until the request is made (i.e. how would we know that it's the testData/* folders for example that are relevant, until we get a request for that... it might as well be the /tmp folder or the like)
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.
or am I missing something vital?
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.
Yes, that's right - we would index the filesystem tree under the mount point on startup
lib/dataSource-recursiveFile.js
Outdated
} | ||
|
||
// resolve path and filename | ||
var resolvedPath = kettle.dataSource.URL.resolveUrl(that.options.path, that.options.termMap, directModel, true).replace("//", "/"); |
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.
We should take the opportunity to factor datasource-file properly - we can share a front-end which does this resolution, and share a back-end which actually sources the data
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'm not sure if this comment is still relevant, since changing the code to do a scan a startup, the path is no longer used and there is generally very little overlap between the two components
@amb26 This is ready for another round of review |
CI job failed: https://ci.fluidproject.org/job/kettle-pull-request/1/ |
|
No description provided.