Improve DirectorySanner to properly handle first file being empty #931
Conversation
smgallo
force-pushed
the
etl/handle-empty-data-file
branch
5 times, most recently
from
be3d949 to
406e15e
Compare
smgallo
force-pushed
the
etl/handle-empty-data-file
branch
from
406e15e to
1ea2e62
Compare
| @@ -98,7 +98,7 @@ protected function _execute() | |||
| // If there are no records we can bail out. Otherwise we may not even be able to infer the | |||
| // source field names. | |||
|
|
|||
| if ( 0 == $this->sourceEndpoint->count() ) { | |||
| if ( false === $firstRecord || 0 == $this->sourceEndpoint->count() ) { | |||
| $options = sprintf('-c %s -a %s %s', $configFile, $action, $localOptions); | ||
|
|
||
| // Add a verbosity flag if the local options do not already contain one | ||
| if ( "" == $localOptions || false === strpos($localOptions, '-v') ) { |
There was a problem hiding this comment.
Do you mean "" == $localOptions? What does that add?
There was a problem hiding this comment.
Just that it would fail if $localOptions was ever anything other than a string. Don't know if that's a concern or not though.
There was a problem hiding this comment.
Since we have complete control over the tests and the default is a string if not specified I think we're fine with a ==.
There was a problem hiding this comment.
The only question / comment I have would be whether or not we ever think we'll need to identify a particular record in either of the two new tables ( generic_cloud_raw_event or generic_cloud_raw_instance_type ). If so, it might be useful to have a generated unique id of some type added. Other then that, LGTM.
|
If we do need to identify a specific record, that would be handled by the ingestor. The DirectoryScanner is just an endpoint for reading the files. |
|
facepalm I may have missed that those are test artifacts... Sorry 'bout that. All good :) |
Description
The DirectoryScanner data endpoint now properly handles the case where the first file scanned is empty or otherwise contains no valid records. Previously, the first record (which is used to determine field names) was set even if the first file had no data and this caused an error.
Motivation and Context
Bugfix
Tests performed
Added new test, manual testing in docker.
Types of changes
Checklist: