[BEAM-2885] Implement a Local Artifact Retrieval service#4422
[BEAM-2885] Implement a Local Artifact Retrieval service#4422tgroh merged 4 commits intoapache:masterfrom
Conversation
tgroh
force-pushed
the
artifact_retrieval_service
branch
from
c69f828 to
f135ab9
Compare
| artifactsDirectory.exists(), "Nonexistent artifact directory %s", artifactsDirectory); | ||
| checkArgument( | ||
| artifactsDirectory.isDirectory(), | ||
| "Artifact Location %s is not a directory", |
| * LocalFileSystemArtifactRetrievalService}. | ||
| */ | ||
| private ArtifactApi.Manifest getManifest() throws IOException { | ||
| return ArtifactApi.Manifest.parseFrom(new FileInputStream(location.getManifestFile())); |
There was a problem hiding this comment.
Lets do this during construction instead of failing during a client call.
| input.read(buf); | ||
| buf.flip(); | ||
| chunks.add(buf); | ||
| } while (chunks.size() * DEFAULT_CHUNK_SIZE < input.size()); |
There was a problem hiding this comment.
This will load everything into memory. Consider using https://docs.oracle.com/javase/8/docs/api/java/nio/channels/FileChannel.html#map-java.nio.channels.FileChannel.MapMode-long-long- and moving the reading part into the getArtifact loop.
| File baseFolder = tmp.newFolder(); | ||
| LocalArtifactStagingLocation newLocation = LocalArtifactStagingLocation.createAt(baseFolder); | ||
| File newManifest = newLocation.getManifestFile(); | ||
| assertThat("Manifest creation failed", newManifest.createNewFile(), is(true)); |
There was a problem hiding this comment.
I would use checkState here instead of assertThat since this is really a precondition to running the test.
You could also use assumeTrue which will mark the test as skipped if the assumption is ever broken but using checkState will force that the test is maintained.
Ditto on other test precondition checks.
assumeTrue works best in cases where you only want to run the test if the system can handle it like only if it can load a JNI library.
There was a problem hiding this comment.
checkState is what we want
tgroh
force-pushed
the
artifact_retrieval_service
branch
from
f135ab9 to
a3c762f
Compare
| return ArtifactApi.Manifest.parseFrom(new FileInputStream(location.getManifestFile())); | ||
| try { | ||
| this.manifest = | ||
| ArtifactApi.Manifest.parseFrom(new FileInputStream(location.getManifestFile())); |
There was a problem hiding this comment.
You need to close the FileInputStream otherwise you'll leak file handles.
tgroh
force-pushed
the
artifact_retrieval_service
branch
3 times, most recently
from
9d51ab3 to
0ef5ad2
Compare
This encapsulates the logic required for the Stager and Retrieval services to use the same directory structure and filenames.
This enables LocalArtifactStagingLocations to be created for a staging location that already exists. This enables retrieval services to be created for a corresponding stager service.
Add a Local Artifact Retrieval Service which implements this interface. The runner is not expected to interact directly with an instance of the ArtifactRetrievalService, and is only exepcted to make it available to environments it creates. As such, the interface is merely a signifier to use within GrpcFnServer.
This mirrors the LocalFileSystemArtifactStagerService, and can be used by containers to retrieve artifacts staged by the pipeline.
tgroh
force-pushed
the
artifact_retrieval_service
branch
from
0ef5ad2 to
7a537b9
Compare
|
run java gradle precommit |
2 participants
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue.mvn clean verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.