Skip to content

Conversation

@maobaolong
Copy link
Member

What changes were proposed in this pull request?

When we deploy a hdfs cluster, our devops usually want to put a file into the cluster and cat the result to ensure anything is ok, so they want this command of ozone too.

Some time, we only want to print the content of the file in Ozone, we don't need to download it now.

What is the link to the Apache JIRA

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

How was this patch tested?

bin/ozone sh key cat /myvol/mybucket/NOTIC.txt

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 @maobaolong for adding this command. It works fine. I have suggestion for a minor improvement in the description. In the future I think we might want to support catting multiple keys (similar to HDFS and Unix cat), but this is a good start.

Copy link
Contributor

@xiaoyuyao xiaoyuyao left a comment

Choose a reason for hiding this comment

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

LGTM overall, just some minor comments inline.

@maobaolong
Copy link
Member Author

@adoroszlai @xiaoyuyao Thanks for your review and help, i've addressed your comments, PTAL.

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 @maobaolong for updating the patch. I only have a question regarding how @xiaoyuyao's comment was addressed.

@maobaolong
Copy link
Member Author

@adoroszlai Thank you for the clarification, i use a const 4096 as chunk size, i think i have address @xiaoyuyao 's comments now, sorry for the misunderstand.

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.

LGTM. @xiaoyuyao please confirm the buffer size, and then I think this can be merged.

@xiaoyuyao
Copy link
Contributor

LGTM, +1 pending CI.

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