HDDS-4357: Rename : make rename an atomic ops by updating key path entry in dir/file table

[rakeshadr]

What changes were proposed in this pull request?

This task is to handle rename key path request and make it an atomic operation by updating the DirTable or FileTable.

What is the link to the Apache JIRA

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

How was this patch tested?

Added TestOzoneFileSystemV1 UT. I will add more unit test cases eventually.

[linyiqun]

Thanks for updating the latest PR, @rakeshadr .
I left my comments below, all of them are for the test.

[bharatviswa504]

Question: What is meant here by should not exist?

[rakeshadr]

For example,
source: /x/y/file1 and destin: /root_dir/file1.renamed, which exits.
Rename checks whether the destin is a file type and if exists then throws KEY_ALREADY_EXISTS exception.

I have checked existing code and followed same behavior.

Does this make sense to you?

[rakeshadr]

@linyiqun @bharatviswa504 thanks for the review help. I've updated PR by addressing your comments, please take another look at it.

[linyiqun]

Seems a very good test coverage now, thanks for adding more testing.
+1.

[bharatviswa504]

Not got when parent is null, why we need to return keyName

[rakeshadr]

This is done to avoid any NPE and I thought below logic will throw exception. Does that make sense to you?

OzoneFileStatus toKeyParentDirStatus = getOMKeyInfoIfExists(metaMgr,
          volumeName, bucketName, toKeyParentDir, 0);
  // check if the immediate parent exists
  if (toKeyParentDirStatus == null || toKeyParentDirStatus.isFile()) {
    throw new OMException(String.format(
            "Failed to rename %s to %s, %s is a file", fromKeyName, toKeyName,
            toKeyParentDir), OMException.ResultCodes.KEY_RENAME_ERROR);
  }

[bharatviswa504]

You mean to say here because we use Paths.get will cause issue here?

[rakeshadr]

Yes, it won't occur in general, just return keyname it for the safer side.

[bharatviswa504]

Thank You @rakeshadr for the update and extensive test coverage.
I have few minor comments/questions.

[rakeshadr]

Thank You @rakeshadr for the update and extensive test coverage.
I have few minor comments/questions.

Thanks a lot @bharatviswa504 for the comments. Updated patch and also replied to your comments.

[bharatviswa504]

+1 LGTM, one question can go ahead with commit.

[rakeshadr]

+1 LGTM, one question can go ahead with commit.

Thanks a lot @bharatviswa504 for the reviews. I replied to that comment. I don't have strong opinion on that but added for safe side. Will revisit that part later, if you disagree.

[rakeshadr]

Thanks @bharatviswa504 and @linyiqun for the detailed reviews.

Regarding the test failure. I looked at the logs and client failed to contact OM server due to NoRouteHostException. I believe, this error is unrelated to the current patch and planning to go ahead with merge.

Caused by: java.net.NoRouteToHostException: No route to host
	at java.base/sun.nio.ch.SocketChannelImpl.checkConnect(Native Method)
	at java.base/sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:779)
	at org.apache.hadoop.net.SocketIOWithTimeout.connect(SocketIOWithTimeout.java:206)
	at org.apache.hadoop.net.NetUtils.connect(NetUtils.java:533)
	at org.apache.hadoop.ipc.Client$Connection.setupConnection(Client.java:700)
	at org.apache.hadoop.ipc.Client$Connection.setupIOstreams(Client.java:804)
	at org.apache.hadoop.ipc.Client$Connection.access$3800(Client.java:421)
	at org.apache.hadoop.ipc.Client.getConnection(Client.java:1606)
	at org.apache.hadoop.ipc.Client.call(Client.java:1435)
	... 35 more
No route to host