Skip to content

Comments

HDDS-9607. Overwrite file by multipart upload, saving wrong ReplicationConfig in KeyInfo#5534

Merged
ChenSammi merged 1 commit intoapache:masterfrom
guohao-rosicky:guohao-HDDS-9607-dev
Nov 8, 2023
Merged

HDDS-9607. Overwrite file by multipart upload, saving wrong ReplicationConfig in KeyInfo#5534
ChenSammi merged 1 commit intoapache:masterfrom
guohao-rosicky:guohao-HDDS-9607-dev

Conversation

@guohao-rosicky
Copy link
Contributor

@guohao-rosicky guohao-rosicky commented Nov 2, 2023

What changes were proposed in this pull request?

When uploading a file with the same name using a slice overwrite, if the number of previous copies is not the same as the current one, the meta information is updated incorrectly.

Example:

  1. a file with the name A is uploaded the first time it is ratis 3 factor
  2. the second time a file with the same name A is uploaded using multipart upload it is ec-3-2-512k
  3. the replication config in the meta-information is recorded as ratis 3 factor, but it should actually be ec-3-2-512k.
# create a ec bucket
$ ozone sh bucket create --layout=OBJECT_STORE  --replication=rs-3-2-512k --replication-type=EC s3v/testbucket

$ ozone sh bucket info s3v/testbucket
{
  "metadata" : { },
  "volumeName" : "s3v",
  "name" : "testbucket",
  "storageType" : "DISK",
  "versioning" : false,
  "usedBytes" : 0,
  "usedNamespace" : 0,
  "creationTime" : "2023-11-08T06:23:39.345Z",
  "modificationTime" : "2023-11-08T06:23:39.345Z",
  "sourcePathExist" : true,
  "quotaInBytes" : -1,
  "quotaInNamespace" : -1,
  "bucketLayout" : "OBJECT_STORE",
  "owner" : "work",
  "replicationConfig" : {
    "data" : 3,
    "parity" : 2,
    "ecChunkSize" : 524288,
    "codec" : "RS",
    "replicationType" : "EC",
    "requiredNodes" : 5
  },
  "link" : false
}

$ dd if=/dev/zero of=./100MB.file count=1024 bs=102400
# first upload a 100MB file use 3 factor
$ ozone sh key put --stream --replication-type=RATIS --replication=THREE  s3v/testbucket/100MB.file ./100MB.file

# keyinfo is well
$ ozone sh key list s3v/testbucket
[ {
  "volumeName" : "s3v",
  "bucketName" : "testbucket",
  "name" : "100MB.file",
  "dataSize" : 104857600,
  "creationTime" : "2023-11-08T06:33:23.236Z",
  "modificationTime" : "2023-11-08T06:33:25.799Z",
  "replicationConfig" : {
    "replicationFactor" : "THREE",
    "requiredNodes" : 3,
    "replicationType" : "RATIS"
  },
  "metadata" : { 
  }
  "file" : false
} ]

# overwrite this file, use MPU
$ aws configure set default.s3.multipart_chunksize 20MB
$ aws s3 --endpoint-url http://127.0.0.1:19878 cp ./100MB.file s3://testbucket/100MB.file

# this keyinfo replicationConfig should be ec-3-2-512k
$ ozone sh key list s3v/testbucket
[ {
  "volumeName" : "s3v",
  "bucketName" : "testbucket",
  "name" : "100MB.file",
  "dataSize" : 104857600,
  "creationTime" : "2023-11-08T06:35:23.146Z",
  "modificationTime" : "2023-11-08T06:35:25.219Z",
  "replicationConfig" : {
    "replicationFactor" : "THREE",
    "requiredNodes" : 3,
    "replicationType" : "RATIS"
  },
  "metadata" : { 
  }
  "file" : false
} ]

After use this PR, overwrite this file, replicationconfig is ec-3-2-512k

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9607

@guohao-rosicky
Copy link
Contributor Author

@kerneltime PTAL. Thanks.

@kerneltime
Copy link
Contributor

Thank you @guohao-rosicky I will take a look. Thank you for catching this.

@kerneltime
Copy link
Contributor

@SaketaChalamchala @tanvipenumudy @xBis7 can you please take a look.

@xBis7
Copy link
Contributor

xBis7 commented Nov 2, 2023

@guohao-rosicky Thanks for the patch. Can you add a comment with a manual test case before and after the change? Also, can we verify it's consistent with FSO as well?

When saying test case, I mean show what the issue is before the change and that it's fixed after the change.

@guohao-rosicky
Copy link
Contributor Author

guohao-rosicky commented Nov 8, 2023

@guohao-rosicky Thanks for the patch. Can you add a comment with a manual test case before and after the change? Also, can we verify it's consistent with FSO as well?

When saying test case, I mean show what the issue is before the change and that it's fixed after the change.

sure. I've updated. Thanks @xBis7 PTAL.

BTW. FSO is extends S3MultipartUploadCompleteRequest, is well.

public class S3MultipartUploadCompleteRequestWithFSO extends S3MultipartUploadCompleteRequest {

@guohao-rosicky guohao-rosicky requested a review from xBis7 November 8, 2023 06:54
Copy link
Contributor

@xBis7 xBis7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guohao-rosicky Thanks for the fix and for updating the comment. Changes LGTM!

Copy link
Contributor

@ChenSammi ChenSammi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good, and the manual test comments are very clear.

Thanks @guohao-rosicky .

@ChenSammi ChenSammi merged commit 585a789 into apache:master Nov 8, 2023
ibrusentsev pushed a commit to ibrusentsev/ozone that referenced this pull request Nov 14, 2023
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Feb 1, 2024
…ng ReplicationConfig in KeyInfo (apache#5534)

(cherry picked from commit 585a789)
Change-Id: I7ae0991740b432c38c9107a805f63523034e1936
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants