Skip to content

Conversation

@ayushtkn
Copy link
Member

What changes were proposed in this pull request?

Added path name validation in OFS

What is the link to the Apache JIRA

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

How was this patch tested?

Added UT

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 @ayushtkn for taking care of this.

path = new Path(workingDir, path);
}
String key = path.toUri().getPath();
if (!OzoneFSUtils.isValidName(path.toUri().getPath())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid unnecessary duplicate conversion:

Suggested change
if (!OzoneFSUtils.isValidName(path.toUri().getPath())) {
if (!OzoneFSUtils.isValidName(key)) {

Comment on lines 817 to 819
if (OzoneFSUtils.isValidName(key)) {
key = path.toUri().getPath();
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not introduced in this patch, but would be nice to remove duplicate conversion:

Suggested change
if (OzoneFSUtils.isValidName(key)) {
key = path.toUri().getPath();
} else {
if (!OzoneFSUtils.isValidName(key)) {

LambdaTestUtils.intercept(InvalidPathException.class, "Invalid path Name",
() -> fs.create(path, false));
} finally {
if (outputStream != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like outputStream is never updated to non-null value, hence unnecessary.

@ayushtkn
Copy link
Member Author

Thanx @adoroszlai for the review, have addressed the above comments

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 @ayushtkn for updating the patch.

@ayushtkn ayushtkn merged commit b80d9b0 into apache:master Nov 14, 2020
@ayushtkn
Copy link
Member Author

Thanx @adoroszlai for the review!!!

errose28 added a commit to errose28/ozone that referenced this pull request Nov 18, 2020
* master: (53 commits)
  HDDS-4458. Fix Max Transaction ID value in OM. (apache#1585)
  HDDS-4442. Disable the location information of audit logger to reduce overhead (apache#1567)
  HDDS-4441. Add metrics for ACL related operations.(Addendum for HA). (apache#1584)
  HDDS-4081. Create ZH translation of StorageContainerManager.md in doc. (apache#1558)
  HDDS-4080. Create ZH translation of OzoneManager.md in doc. (apache#1541)
  HDDS-4079. Create ZH translation of Containers.md in doc. (apache#1539)
  HDDS-4184. Add Features menu for Chinese document. (apache#1547)
  HDDS-4235. Ozone client FS path validation is not present in OFS. (apache#1582)
  HDDS-4338. Fix the issue that SCM web UI banner shows "HDFS SCM". (apache#1583)
  HDDS-4337. Implement RocksDB options cache for new datanode DB utilities. (apache#1544)
  HDDS-4083. Create ZH translation of Recon.md in doc (apache#1575)
  HDDS-4453. Replicate closed container for random selected datanodes. (apache#1574)
  HDDS-4408: terminate Datanode when Datanode State Machine Thread got uncaught exception. (apache#1533)
  HDDS-4443. Recon: Using Mysql database throws exception and fails startup (apache#1570)
  HDDS-4315. Use Epoch to generate unique ObjectIDs (apache#1480)
  HDDS-4455. Fix typo in README.md doc (apache#1578)
  HDDS-4441. Add metrics for ACL related operations. (apache#1571)
  HDDS-4437. Avoid unnecessary builder conversion in setting volume Quota/Owner request (apache#1564)
  HDDS-4417. Simplify Ozone client code with configuration object (apache#1542)
  HDDS-4363. Add metric to track the number of RocksDB open/close operations. (apache#1530)
  ...
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