s3 based case store #11
Conversation
Signed-off-by: HARPER Jon <[email protected]>
|
SonarCloud Quality Gate failed.
|
src/main/java/com/powsybl/caseserver/datasource/util/S3CaseDataSourceService.java
Outdated
Show resolved
Hide resolved
src/test/java/com/powsybl/caseserver/datasource/util/AbstractCaseDataSourceControllerTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/powsybl/caseserver/datasource/util/AbstractCaseDataSourceControllerTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Rehili Ghazwa <[email protected]>
Signed-off-by: Rehili Ghazwa <[email protected]>
Signed-off-by: Rehili Ghazwa <[email protected]>
Signed-off-by: Rehili Ghazwa <[email protected]>
Signed-off-by: Rehili Ghazwa <[email protected]>
Signed-off-by: Rehili Ghazwa <[email protected]>
Signed-off-by: Rehili Ghazwa <[email protected]>
Signed-off-by: Rehili Ghazwa <[email protected]>
Signed-off-by: Rehili Ghazwa <[email protected]>
Signed-off-by: Rehili Ghazwa <[email protected]>
Signed-off-by: Rehili Ghazwa <[email protected]>
src/test/java/com/powsybl/caseserver/AbstractContainerConfig.java
Outdated
Show resolved
Hide resolved
| aws: | ||
| s3: | ||
| # using host ip address for s3 endpoint because spring cloud aws 2.x doesn't allow | ||
| # spring.cloud.aws.s3.path-style-access-enabled (only spring cloud aws 3.x), to be revisisted ? |
There was a problem hiding this comment.
can this be revisted now ?
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
| return new CaseException(Type.TEMP_FILE_PROCESS, "Error processing temporary case file: " + uuid, e); | ||
| } | ||
|
|
||
| public static CaseException importZipContent(UUID uuid, Exception e) { |
There was a problem hiding this comment.
importZipContent and processTempFile unused to be removed
| } | ||
|
|
||
| public Set<String> listName(UUID caseUuid, String regex) { | ||
| List<String> fileNames; |
There was a problem hiding this comment.
redundancy calls of getOriginalFilename
String originalFilename = getOriginalFilename(caseUuid);
| List<String> fileNames; | ||
| if (isCompressedCaseFile(getOriginalFilename(caseUuid))) { | ||
| // For a compressed file basename.xml.gz, listName() should return ['basename.xml']. That's why we remove the compression extension to the filename. | ||
| fileNames = List.of(removeExtension(getOriginalFilename(caseUuid), "." + getCompressionFormat(caseUuid))); |
There was a problem hiding this comment.
to eliminate the need for if-else then if ...
return Set.of(removeExtension(originalFilename, "." + getCompressionFormat(caseUuid)))
.stream()
.filter(name -> name.matches(regex))
.collect(Collectors.toSet());
There was a problem hiding this comment.
Yes but the actual form of the code allow to filter with the regex at only one place and the logic is easier to comprehend that way
| fileNames = List.of(removeExtension(getOriginalFilename(caseUuid), "." + getCompressionFormat(caseUuid))); | ||
| } else { | ||
| List<S3Object> s3Objects = getCaseFileSummaries(caseUuid); | ||
| fileNames = s3Objects.stream().map(obj -> Paths.get(obj.key()).toString().replace(CASES_PREFIX + caseUuid.toString() + DELIMITER, "")).toList(); |
There was a problem hiding this comment.
use getCaseName(caseUuid) to replace Paths.get(obj.key()).toString().replace(CASES_PREFIX + caseUuid.toString() + DELIMITER, "")
There was a problem hiding this comment.
I don't get what you want to do. Here, we need to clean the key from the bucket name and the caseUuid. We can't do much with getCaseName(caseUuid)
| // the original archive name has to be filtered. | ||
| fileNames = fileNames.stream().filter(name -> !name.equals(getOriginalFilename(caseUuid))).toList(); | ||
| // each subfile hase been gzipped -> we have to remove the gz extension (only one, the one we added). | ||
| fileNames = fileNames.stream().map(name -> removeExtension(name, GZIP_EXTENSION)).toList(); |
There was a problem hiding this comment.
fileNames = fileNames.stream()
.filter(name -> !name.equals(originalFilename))
.map(name -> removeExtension(name, GZIP_EXTENSION))
.collect(Collectors.toList());
| // To optimize copy, files to copy are not downloaded on the case-server. They are directly copied on the S3 server. | ||
| CopyObjectRequest copyObjectRequest = CopyObjectRequest.builder() | ||
| .sourceBucket(bucketName) | ||
| .sourceKey(CASES_PREFIX + sourcecaseUuid + "/" + fileName) |
| .sourceBucket(bucketName) | ||
| .sourceKey(CASES_PREFIX + sourcecaseUuid + "/" + fileName) | ||
| .destinationBucket(bucketName) | ||
| .destinationKey(CASES_PREFIX + caseUuid + "/" + fileName) |
| try { | ||
| s3Client.copyObject(copyObjectRequest); | ||
| } catch (S3Exception e) { | ||
| throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Source file " + caseUuid + "/" + fileName + NOT_FOUND); |
| if (!objectsToDelete.isEmpty()) { | ||
| DeleteObjectsRequest deleteObjectsRequest = DeleteObjectsRequest.builder() | ||
| .bucket(bucketName) | ||
| .delete(Delete.builder().objects(objectsToDelete).build()) |
There was a problem hiding this comment.
.delete(delete -> delete.objects(objectsToDelete))
| if (!objectsToDelete.isEmpty()) { | ||
| DeleteObjectsRequest deleteObjectsRequest = DeleteObjectsRequest.builder() | ||
| .bucket(bucketName) | ||
| .delete(Delete.builder().objects(objectsToDelete).build()) |
There was a problem hiding this comment.
.delete(delete -> delete.objects(objectsToDelete))
| int firstSlash = key.indexOf(DELIMITER); | ||
| int secondSlash = key.indexOf(DELIMITER, firstSlash + 1); | ||
| return key.substring(secondSlash + 1); | ||
| } |
There was a problem hiding this comment.
Both methods parseUuidFromKey and parseFilenameFromKey are finding the indices of firstSlash and secondSlash, which are the same calculations in both. We can calculate these indices only once and reuse them
There was a problem hiding this comment.
not sure if it's worth it. If we wanted to do this, we should create a pojo with all the parsed info and then someone can read the differents parts..
There was a problem hiding this comment.
Yes too complicated. let's keep it simple
| return getCaseMetaDataEntity(caseUuid).getOriginalFilename(); | ||
| } | ||
|
|
||
| // key format is "gsi-cases/UUID/filename" |
There was a problem hiding this comment.
| // key format is "gsi-cases/UUID/filename" | |
| // key format is "gsi-cases/UUID/path/to/file" |
|
|
||
| private List<S3Object> getCaseFileSummaries(UUID caseUuid) { | ||
| List<S3Object> files = getCasesSummaries(uuidToKeyPrefix(caseUuid)); | ||
| if (files.size() > 1) { |
| return files; | ||
| } | ||
|
|
||
| private List<CaseInfos> infosFromDownloadCaseFileSummaries(List<S3Object> objectSummaries) { |
There was a problem hiding this comment.
rename remove "Download" we should use it only for fileaccess to better understand the code ?
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
…ginalFilename() once Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
Signed-off-by: Etienne Homer <[email protected]>
|
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
NO
What kind of change does this PR introduce?
Feature
What is the current behavior?
cases stored in folder
What is the new behavior (if this is a feature change)?
cases stored in external s3 api
Does this PR introduce a breaking change or deprecate an API?
NO
Other information:
not finished, need all API to be efficient. For now everything is downloaded everytime