BulkServiceManager should delete its temp files

[markus-s24]

The BulkServiceManager should take care that all its temp files are deleted.

Especially uploadEntitiesAsync() should delete its temp file after the upload request succeeded. downloadEntitiesAsync() should return a BulkEntityIterable that deletes its underlying file on close().

I known that I can use cleanupTempFiles(), but that is no good option, because that tries to delete files generated by other BulkServiceManagers too.

[shyTNT]

Thanks @markus-s24 . These temp files can track the state of upload and download operation. For example, the upload result file will record the error information of upload operation. So it is not very appropriate to delete all the temp files. To avoid deleting files generated by other BulkServiceManager, you can set workingDirectory for each BulkServiceManager and cleanupTempFiles will delete their own temp files.

[dennis-yemelyanov]

Yes, as @shyTNT pointed out, initially we wanted to keep them to make troubleshooting easier on the client side (so they basically act as log files), but deleting them automatically would also be useful for some scenarios. Maybe we can make this configurable by adding a flag cleanupTempFilesAutomatically to BulkServiceManager? For now we can set it to 'false' by default to keep existing behavior and think more about if we want to change the default for this setting later.

[markus-s24]

Thanks @shyTNT for pointing out, that I can use a different temp dir for each BulkServiceManager instance. That helped me keeping my temp directories clean, though I need to write some cleanup code.

Anyway it would be nicer, if BulkServiceManager deletes them, if they are no longer used. That way I do not need to clean them up myself. I issued the PR https://github.com/BingAds/BingAds-Java-SDK/pull/42 that includes the suggested changes at least for the entity based upload and download methods.

As for the default for cleanupTempFilesAutomatically I would of suggest to set it to true, because setting it to false is just for debugging. BTW the on the fly created temp zip file already will be deleted automatically. But I won't be disappointed if this functionality makes it into BulkServiceManager with the default set to false ;-)

I added this flag to my PR, I hope you like it.

[dennis-yemelyanov]

Thank you, Markus, for making this change. I'm fine to merge it, and we can probably set the default value to true in the next major update. Jianzhong, do you have any concerns?

[shyTNT]

Thank you, @markus-s24. The default value to true may bring some unexpected changes for our users.

[shyTNT]

Hi, @markus-s24. I tested your PR and found that the BulkEntityIterable returned when upload/download entities can't go through the override method of close() in CSVBulkFileReaderFactory. This is because the override close() method is for BulkFileReader rather than the close() method in BulkEntityIterable.

[markus-s24]

Sorry for the delay, I will redo the PR.

[markus-s24]

See https://github.com/BingAds/BingAds-Java-SDK/pull/45

[eric-urban]

Hi @markus-s24

Just wanted to provide an update i.e. we tentatively plan to address this before end of June.

[eric-urban]

Hi @markus-s24

The team is actively evaluating and prototyping this update. However, we will not make it into this week's release. Will provide another update once we have a confirmed ETA.

[eric-urban]

This is resolved with release 11.5.5. Please reach out if you find any issue.