Fix corrupted archive with checksum validation, synchronized lock and API enhancement #51
Conversation
| long currentBlock = Math.min( remaining, BOLCK_SIZE ); | ||
| MappedByteBuffer buffer = channel.map( FileChannel.MapMode.READ_ONLY, position, currentBlock ); | ||
| digest.update( buffer ); | ||
| position += currentBlock; |
There was a problem hiding this comment.
Is there any 3rd lib for this? like e.g.: commons-codec .
There was a problem hiding this comment.
@sswguo done in new commit.
| @@ -79,6 +90,35 @@ public class ArchiveController | |||
|
|
|||
| private final String PART_ARCHIVE_SUFFIX = PART_SUFFIX + ARCHIVE_SUFFIX; | |||
|
|
|||
| private static final int threads = 4 * Runtime.getRuntime().availableProcessors(); | |||
There was a problem hiding this comment.
maybe we can make it configurable.
There was a problem hiding this comment.
@sswguo sure, done in new commit.
yma96
changed the title
| { | ||
| while ( isInProgress( buildConfigId ) ) | ||
| { | ||
| logger.info( "There is already generation process in progress for buildConfigId {}, try lock wait.", |
There was a problem hiding this comment.
can we just throw warning to user that there is already process in progress, please try later ? sorry if I miss some requirements. ; -)
There was a problem hiding this comment.
@sswguo You mean don't let the coming process wait, just warn to user and return? Not sure whether PNC care the response of archive generation since this should be at the end of build, if that's the point I will comment to confirm.
There was a problem hiding this comment.
We can confirm that which case will send two same archive requests in a short time.
There was a problem hiding this comment.
@sswguo probably large build with large archive file, since PNC UI doesn't allow concurrent builds with the same config ID, the previous build might be already done but generation with download was still in progress, then another build was in progress and used the same workspace to process download or generation, so we need to know if they want to handle the generation response themselves or just let archive go with the right thing, comment on JIRA and discuss on meeting.
|
LGTM. |
| + " \"path\": \"\"," + " \"md5\": \"\"," + " \"sha256\": \"\"," | ||
| + " \"sha1\": \"\"," + " \"size\": 001" + " }," + "..." | ||
| + "]}", schema = @Schema( implementation = HistoricalContentDTO.class ) ) ) | ||
| @APIResponse( responseCode = "409", description = "The archive created request is conflicted" ) |
There was a problem hiding this comment.
Not sure if 409 is good response to PNC as this may fail the request. We should let them know if we agree to use this.
There was a problem hiding this comment.
Can't add @michalovjan to review, could you help to confirm on this? Also comment on JIRA https://issues.redhat.com/browse/MMENG-4256.
There was a problem hiding this comment.
Hi @yma96, I'd rather have just 202 Accepted if that's okay.
But if you're keen on having 409, we'd need to add that to repository-driver so that we don't unnecessarily retry requests. It's doable.
There was a problem hiding this comment.
@michalovjan make sense, I'll change to 202 accepted to avoid more code change from PNC.
|
Merge this and prepare devel test, leave comment for any other concern. |
This fix the bugs:
https://issues.redhat.com/browse/MMENG-4251
https://issues.redhat.com/browse/MMENG-4256
https://issues.redhat.com/browse/MMENG-4257
Already test locally and validation pass.