Skip to content

Conversation

@elek
Copy link
Member

@elek elek commented Nov 30, 2020

What changes were proposed in this pull request?

Create new Freon test for the closed container replication.

What is the link to the Apache JIRA

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

How was this patch tested?

#Generate keys
ozone ockg -n 10
#Close container
ozone container close 1
#Execute freon
ozone freon cr

Containers supposed to be visible under /tmp/....

Note: this patch requires the changes in #1551

@elek elek requested a review from xiaoyuyao November 30, 2020 13:49
@elek elek changed the title HDDS-4496. Separate client and server2server GRPC services of datanode HDDS-4524. Create freon test to measure closed container replication Nov 30, 2020
@elek elek requested review from jojochuang and removed request for xiaoyuyao December 1, 2020 08:12
@adoroszlai
Copy link
Contributor

adoroszlai commented Dec 7, 2020

Containers supposed to be visible under /tmp

I think this depends on hdds.datanode.dir setting. In ozone compose env the container was replicated by default to /data/hdds. Got it to replicate to /tmp with: ozone freon -Dhdds.datanode.dir=/tmp cr -t 1.

On another note, trying to run ozone freon cr the second time leads to:

[main] ERROR volume.MutableVolumeSet: Failed to parse the storage location: /tmp
java.io.IOException: Volume is in an INCONSISTENT state. Skipped loading volume: /tmp/hdds
  at org.apache.hadoop.ozone.container.common.volume.HddsVolume.initialize(HddsVolume.java:225)

Is this expected? Do you know a workaround for that (other than deleting /tmp/hdds)?

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just some nits

@elek
Copy link
Member Author

elek commented Jan 6, 2021

Is this expected? Do you know a workaround for that (other than deleting /tmp/hdds)?

I can implement an automatic delete, but I think it's more safe to live it for the users. It seems to be more safe to fail when the directory is not empty (harder to understand what was downloaded) and ask users to delete the dir.

But I am not sure, can be convinced.

@elek
Copy link
Member Author

elek commented Jan 6, 2021

@jojochuang Thanks for the suggestions, I updated the branch with them.

@adoroszlai
Copy link
Contributor

I can implement an automatic delete, but I think it's more safe to live it for the users. It seems to be more safe to fail when the directory is not empty (harder to understand what was downloaded) and ask users to delete the dir.

Agreed. Can you add a comment somewhere to remind users to delete the dir in case of this error?

@elek
Copy link
Member Author

elek commented Jan 6, 2021

I can implement an automatic delete, but I think it's more safe to live it for the users. It seems to be more safe to fail when the directory is not empty (harder to understand what was downloaded) and ask users to delete the dir.

Agreed. Can you add a comment somewhere to remind users to delete the dir in case of this error?

Sure. I added a check to return with more meaningful error. (5aa2fcc)

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @elek for updating the patch.

@elek
Copy link
Member Author

elek commented Jan 10, 2021

Merged. Thanks the review @adoroszlai and @jojochuang

@elek elek merged commit 247dad5 into apache:master Jan 10, 2021
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.

3 participants