[RSDK-1616] Do not attempt to sync empty sensor reading files #1716
Conversation
viambot
added
the
safe to test
|
@AaronCasas Made this specific to data capture files. If a user tries to upload an empty arbitrary file, I think reasonable to error out and indicate that something's wrong since those are expected to be well-formed and not continuously appended to. Adding a separate one-off sync_test for this right now. |
|
@agreenb I think it's actually valid to upload an empty arbitrary file, and we don't treat this as an error app side. Reason being that if the cloud is meant to be a mirror of what was on the robot, ignoring empty files leaves ambiguity: if I don't see |
| @@ -18,6 +18,11 @@ func uploadDataCaptureFile(ctx context.Context, client v1.DataSyncServiceClient, | |||
| return err | |||
| } | |||
|
|
|||
| // Do not attempt to upload a file without any sensor readings, which occurs when syncing more frequently than capturing. | |||
There was a problem hiding this comment.
I think the post , part can be left out, since there are other cases where this would also apply (e.g. restarting/disabling/changing before first write)
viambot
added
safe to test
viambot
added
safe to test
| @@ -190,6 +192,10 @@ func TestDataCaptureUpload(t *testing.T) { | |||
| dataType: v1.DataType_DATA_TYPE_BINARY_SENSOR, | |||
| numFails: 2, | |||
| }, | |||
| { | |||
| name: "If syncing more often than capturing, empty sensor reading files should be deleted locally and not uploaded.", | |||
There was a problem hiding this comment.
Sort of echoing other comment: I think the more general rule here is just about a file having no sensor data vs this relationship between capture and sync rate, so something like "Files with no sensor data should not be synced" would be a bit more informative
Thanks for linking, and the reasoning makes sense and I'd actually agree. The difference in data capture upload is that the files without sensor readings aren't useful or queryable, and we know exactly why they'd be empty — syncing too frequently or the reasons you listed like restarting/disabling/changing. |
viambot
added
safe to test
| @@ -289,6 +303,10 @@ func TestDataCaptureUpload(t *testing.T) { | |||
| } | |||
| failedURs := mockService.getFailedDCUploadRequests() | |||
|
|
|||
| if tc.emptyFile { | |||
There was a problem hiding this comment.
Is this not captured by the check below already? Since len(capturedData) should equal 0 in this case (based on above)
viambot
added
safe to test
|
agreenb
changed the title
Empty sensor readings occur when:
This will also delete the local empty file.