Skip to content

Conversation

@adoroszlai
Copy link
Contributor

What changes were proposed in this pull request?

Reject rewriteKey call at client if OM does not support it.

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

How was this patch tested?

Created command ozone sh key rewrite to call the new API, toggling replication between RATIS/3 and EC/3,2.

Tested the command in ozone environment to verify it works:

$ cd hadoop-ozone/dist/target/ozone-1.5.0-SNAPSHOT/compose/ozone
$ OZONE_DATANODES=5 ./run.sh -d
$ docker-compose exec scm bash
bash-4.2$ ozone freon ockg -n1 -t1 # create volume and bucket
...
bash-4.2$ ozone sh key put /vol1/bucket1/rocksdbjni-7.7.3.jar share/ozone/lib/rocksdbjni-7.7.3.jar
bash-4.2$ ozone sh key list /vol1/bucket1 -p=rocksdbjni-7.7.3.jar 
[ {
  "creationTime" : "2024-05-16T18:33:10.860Z",
  "modificationTime" : "2024-05-16T18:33:12.277Z",
  "replicationConfig" : {
    "replicationFactor" : "THREE",
    "requiredNodes" : 3,
    "replicationType" : "RATIS"
  },
  ...
} ]
bash-4.2$ ozone sh key rewrite /vol1/bucket1/rocksdbjni-7.7.3.jar 
bash-4.2$ ozone sh key list /vol1/bucket1 -p=rocksdbjni-7.7.3.jar 
[ {
  "creationTime" : "2024-05-16T18:33:10.860Z",
  "modificationTime" : "2024-05-16T18:33:48.322Z",
  "replicationConfig" : {
    "data" : 3,
    "parity" : 2,
    "ecChunkSize" : 1048576,
    "codec" : "RS",
    "requiredNodes" : 5,
    "replicationType" : "EC"
  },
  ...
} ]
bash-4.2$ ozone sh key rewrite /vol1/bucket1/rocksdbjni-7.7.3.jar 
bash-4.2$ ozone sh key list /vol1/bucket1 -p=rocksdbjni-7.7.3.jar 
[ {
  "creationTime" : "2024-05-16T18:33:10.860Z",
  "modificationTime" : "2024-05-16T18:33:55.015Z",
  "replicationConfig" : {
    "replicationFactor" : "THREE",
    "requiredNodes" : 3,
    "replicationType" : "RATIS"
  },
  ...
} ]

Tested the command in xcompat environment with old cluster and new client to verify the change in this PR:

$ cd hadoop-ozone/dist/target/ozone-1.5.0-SNAPSHOT/compose/xcompat
$ OZONE_VERSION=1.4.0 COMPOSE_FILE=old-cluster.yaml:clients.yaml docker-compose up -d --scale datanode=5
$ OZONE_VERSION=1.4.0 COMPOSE_FILE=old-cluster.yaml:clients.yaml docker-compose exec new_client bash
bash-4.2$ ozone freon ockg -n1 -t1 # create volume and bucket
...
bash-4.2$ ozone sh key put /vol1/bucket1/rocksdbjni-7.7.3.jar share/ozone/lib/rocksdbjni-7.7.3.jar
bash-4.2$ ozone sh key rewrite /vol1/bucket1/rocksdbjni-7.7.3.jar 
OzoneManager does not support atomic key rewrite.

The new command is not part of the PR, but can be added if you think it's useful.

CI:
https://github.com/adoroszlai/ozone/actions/runs/9114412166

@adoroszlai adoroszlai self-assigned this May 16, 2024
@adoroszlai adoroszlai requested a review from sodonnel May 17, 2024 07:47
Copy link
Contributor

@sodonnel sodonnel 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.

The rewrite command also looks like a good addition. It feels a bit strange that it just toggles the rep-config without giving it any parameters though. I wonder if it would make sense to pass a new rep-config. If its the same as existing it would error. If its different then it performs the rewrite?

The general concept of rewrite is not only to change the replication config, but to replace a key atomically, perhaps with the same rep-config but different content (eg a json document that has been edited in some way after loading it). It may be slightly tricky to do that with a command like this. I am not sure we want to have to pass a generation on the command line?

Eg, the usage pattern would be some sort of:

  1. Listkey / read key to get the existing key meta data and content
  2. ozone sh rewrite key --expectedGeneration xxx --replication --type key

The optional content and expectedGeneration would only be needed if the content was to be changed. Otherwise you could just call:

ozone sh key rewrite --replication --type /existing/key

We should consider this in another PR, rather than complicating this one with it.

@adoroszlai
Copy link
Contributor Author

It feels a bit strange that it just toggles the rep-config without giving it any parameters though.

I simplified the description a bit. :) The command has the same options for replication as key put. Since they are optional, the default behavior is just toggling between these two. This behavior is geared towards testing, not for final prod.

optional content and expectedGeneration would only be needed if the content was to be changed

I think expectedGeneration could be added to key put instead, which already has argument for content to be uploaded, and already overwrites key if it exists. The new option would be a safety net, similar to git push --force-with-lease.

key rewrite would be a simpler "just change replication to ...".

@sodonnel
Copy link
Contributor

Ah ok - that makes sense. In that case, adding in the rewrite command for debugging makes sense to me. It could be useful as we work on developing the rest of the feature.

LIGHTWEIGHT_LIST_KEYS(4, "OzoneManager version that supports lightweight"
+ " listKeys API."),

ATOMIC_REWRITE_KEY(5, "OzoneManager version that supports rewriting key as atomic operation"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll need to update the specific version number later, as 5 is being added for HDDS-10435.

@adoroszlai adoroszlai merged commit 70df306 into apache:HDDS-10656-atomic-key-overwrite May 17, 2024
@adoroszlai adoroszlai deleted the HDDS-10865 branch May 17, 2024 10:51
@adoroszlai
Copy link
Contributor Author

Thanks @sodonnel for the review.

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.

2 participants