Uploader refactoring and additional attempts for immediate uploaders #1724
Conversation
| private final List<S3UploadMetadata> immediateUploadMetadata; | ||
| private final ReentrantLock lock; | ||
| private final ConcurrentMap<SingularityUploader, Future<Integer>> immediateUploaders; | ||
| private final Map<S3UploadMetadata, SingularityUploader> metadataToimmediateUploader; |
There was a problem hiding this comment.
nit metadataToimmediateUploader -> metadataToImmediateUploader (capitalize immediate)
| LOG.debug("Retrying immediate uploader {}", uploaderMetadata); | ||
| performImmediateUpload(uploader); | ||
| } else { | ||
| LOG.debug("Uploader for metadata {} not found to retry, recreating", uploaderMetadata); |
There was a problem hiding this comment.
not sure I understand this debug line. What is being re-created?
There was a problem hiding this comment.
Ah, copy-pasta error. the ,recreating should be removed, if we don't find the uploader in the map it means it somehow got added to both toRemove and toRetry, which theoretically should never happen, but better than throwing a NPE, updated the message
| SingularityUploader uploader = metadataToimmediateUploader.remove(uploaderMetadata); | ||
| if (uploader != null) { | ||
| LOG.debug("Retrying immediate uploader {}", uploaderMetadata); | ||
| performImmediateUpload(uploader); |
There was a problem hiding this comment.
For this retry loop, the uploader is removed from metadataToImmeditateUploader and when it's called to upload immediately, it'll be added to the immediteUploadersFutures map. During the next run of checkUploads, won't the uploader be missing from metadataToImmediateUploader and cause the check to skip adding the uploadedFiles to the total count? Instead it'll be moved to the toRemove list and hit the 30 second check
There was a problem hiding this comment.
good catch. It ends up working out that it still retries, but that should be a .get so we can do the full retry -> toRemove loop . Fixed
|
lgtm 🚢 |
@darcatron I'm folding #1714 into this as well to avoid too many merge conflicts. Will close the other when they are both in hs_qa
There were a few cases where an immediate uploader could miss files. Particularly, if an uploader was immediate, but then something attempted to recreate it (wrote the file again), before the original one expired and was removed. This updates the uploader driver to: