DS-3978: BitStore support for Azure Blob Storage Service. #2140
Conversation
|
A working sample can be found here: Both attachments (bitstreams) and solr indexing thumbnails goes to my Storage Account as a blob storage. Please do let me know if there is anything else I must provide. |
There was a problem hiding this comment.
The overall feature here looks fine to me. However, I'd like to see other institutions express an interest in adding an Azure Storage Service before merging. Obviously, any new storage service will need to be maintained over time, and if we don't have many active developers who use the storage service, that may make maintenance very difficult.
All that said, I've added inline comments where this code could be improved.
dspace-api/src/main/java/org/dspace/storage/bitstore/AzureBlobStorageBitStoreService.java
Show resolved
Hide resolved
| scratchFile.delete(); | ||
|
|
||
| } catch (Throwable t) { | ||
| log.error("put(" + bitstream.getInternalId() + ", is)", t); |
There was a problem hiding this comment.
It would be best to catch specific Exception types that may be thrown, instead of generically catching Throwable. Catching specific Exception types would allow you to log a more detailed/useful error message, which is what we tend to encourage. The current log.error line here is uninformative, as it just notes the method name -- and that could be easily found out by the stacktrace itself.
| CloudBlob blob = this.blobContainer.getBlockBlobReference(key); | ||
| return (blob != null) ? blob.openInputStream() : null; | ||
| } catch (Exception e) { | ||
| log.error("get(" + key + ")", e); |
There was a problem hiding this comment.
This log.error is uninformative. Should provide more information about the error occurring here, and not just provide the method name.
| t.printStackTrace(printWriter); | ||
| if (t instanceof StorageException) { | ||
| if (((StorageException) t).getExtendedErrorInformation() != null) { | ||
| log.error(String.format("\nError: %s", |
There was a problem hiding this comment.
Instead of this if, this should be a separate catch statement. Again, the log.error should provide more information about why a StorageException error may occur here.
| ((StorageException) t).getExtendedErrorInformation().getErrorMessage()), t); | ||
| } | ||
| } | ||
| log.error(String.format("Exception details:\n%s", stringWriter.toString()), t); |
There was a problem hiding this comment.
Again, need a better error log statement
| } | ||
| } | ||
| log.error(String.format("Exception details:\n%s", stringWriter.toString()), t); | ||
| throw new IOException(t); |
There was a problem hiding this comment.
Similar to all the log.error comments, is there more information we should add to this newly thrown IOException?
|
|
||
| } catch (Exception e) { | ||
| log.error("about(" + key + ", attrs)", e); | ||
| throw new IOException(e); |
There was a problem hiding this comment.
Similar to all the log.error comments, is there more information we should add to this newly thrown IOException?
| } | ||
| } catch (Exception e) { | ||
| log.error("remove(" + key + ")", e); | ||
| throw new IOException(e); |
There was a problem hiding this comment.
Similar to all the log.error comments, is there more information we should add to this newly thrown IOException?
| // Commit the blocks | ||
| blockBlob.commitBlockList(blockList); | ||
| } catch (Throwable t) { | ||
| throw t; |
There was a problem hiding this comment.
This catch doesn't seem to be needed. It's just re-throwing the same exception.
| * | ||
| * @throws Throwable | ||
| */ | ||
| private void uploadFileBlocksAsBlockBlob(CloudBlockBlob blockBlob, String filePath) throws Throwable { |
There was a problem hiding this comment.
throws Throwable is not recommended. I'd rather see several throws here with the individual exception types listed out (and then the Javadocs can note when specific Exception types may occur)
tdonohue
changed the title
github-actions
bot
added
the
merge conflict
|
Due to a family health issue at the time apparently these recommendations and e-mails were not of my awareness and given that the storage engine was even suggested in an old release, I will close this PR and will work on something in the newer releases should we feel it is appropriate. Thanks and sorry for any inconvenience. |
I've implemented the Bitstore support for Azure Blob Storage Service.
https://jira.duraspace.org/browse/DS-3978