Proper handling of exceptions when exporting metadata#8782
Conversation
…e export has succeeded. (#8484)
| ExportService instance = ExportService.getInstance(); | ||
| instance.exportAllFormats(dataset); | ||
|
|
||
| ctxt.datasets().updateLastExportTimeStamp(dataset.getId()); |
There was a problem hiding this comment.
Why both this and setting the update time in exportAllFormats?
Could/should it just be removed here? (Or is a ctxt.datasets().merge(dataset); or similar needed to get the update from the ExportService? )
Can you even get rid of updateLastExportTimeStamp? (I see it called three places, all after a call to exportAllFormats (andexportAllFormats is called a few more places where updateLastExportTimeStamp isn't called)).
There was a problem hiding this comment.
I'm a bit stumped by this actually... With the line 264 above commented out the time stamp does not get updated. And I can't immediately tell why. Is there still something finicky about the transaction under which onSuccess() is called, in this context, of FinalizeDatasetPublicationCommand being executed as a "nested" command?
Not sure about .merge(dataset), why it would be needed - as opposed to having all the changes done to the dataset getting saved automatically at the end of the transaction. ExportService is not an EJB itself, despite the name.
OK, I would need to experiment some more to figure this out.
There was a problem hiding this comment.
Uhm, I would like to just declare this out of scope - figuring out why that line is necessary. It has been there for a while.
There was a problem hiding this comment.
I'd suggest at least a comment since you found it isn't doing anything / doesn't appear to anyway.
There was a problem hiding this comment.
"isn't doing anything"?
Oh, you mean, not the line highlighted above, but the .setLastExportTime() line inside the ExportService.exportAllFormats() method (?).
Sure, I can add a comment here saying that "exportAllFormats() tries to set the last export timestamp; but for some reason that modification isn't getting saved when exportAll is called from here (transaction?), hence this explicit .updateLastExportTimeStamp() is necessary".
There was a problem hiding this comment.
Actually, an explicit .merge() call there makes calling that method unnecessary.
Guessing it may be possible to get rid of it in the other 2 places.
…() in the dataset service bean. (#8484)
5 participants
What this PR does / why we need it:
This is a simple PR that fixes exception handling during metadata exports, making sure the "last exported" time stamp does not get updated unless it was successful.
Which issue(s) this PR closes:
Closes #8484
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: