Implement support for Azure blob storage. #167
Conversation
| throw new UnsupportedOperationException("Not yet implemented"); | ||
| try { | ||
| CloudBlobContainer container = blobClient.getContainerReference(jobId.toString()); | ||
| container.createIfNotExists(); |
There was a problem hiding this comment.
Can this throw an IOException if a job already exists with the current ID? that is consistent with the docs for createJob(UUID jobId, PortabilityJob job) as well as GoogleJobStore's impl
There was a problem hiding this comment.
I hate to bring this up but has the discussion of checked vs. unchecked exceptions come up? (ducking) :-)
I would argue for unchecked exceptions except in cases where it is absolutely necessary to signal a condition is recoverable. Runtime exceptions can bubble up to the top of the stack where they can be trapped and logged. In the case of a duplicate ID, it seems like an unrecoverable programming error.
What do think?
I'm fine with using checked exceptions if that is what the project standard is as well.
There was a problem hiding this comment.
No that's a great point! I don't think we had talked about it. That convention sounds reasonable to me and looks to be the recommendation in Effective Java.
For now do you want to make it throw an IOException just to be consistent, but leave a TODO/issue to follow up on our convention project-wide?
Or if you want to make it throw an unchecked exception, and change the other create() method to do the same, that's fine with me too.
There was a problem hiding this comment.
I looked at changing the method (and others on the interface) to throw an IOException (or unchecked exception) but that change will impact other classes that use it (e.g. the MS importers and exporters for calendars and contacts) which is probably too much to put in this PR.
I'll raise this as an issue on Slack for people to discuss since others may have opinions. Based on that, I can create another PR to migrate the codebase to whatever approach is decided. In the meantime, I'll file an issue to track as well.
There was a problem hiding this comment.
OK thanks, that sounds good. Could you also either throw an unchecked exception in that place now, since it shouldn't impact other classes, or leave a TODO to throw when the job already exists, just so we don't forget? Thanks!
jimmarino
force-pushed
the
calendar-updates
branch
from
9c5dbe1
to
05fa90e
Compare
jimmarino
force-pushed
the
calendar-updates
branch
from
05fa90e
to
d40a421
Compare
No description provided.