-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDDS-1382. Create customized CSI server for Ozone. #693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
hadoop-ozone/csi/src/main/java/org/apache/hadoop/ozone/csi/CsiServer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make 1000_000_000 configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: LOG.info("Executing {}", mountCommand);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: LOG.info("Executing {}", umountCommand);
|
Thanks @elek for working on this, the change LGTM overall, just few minor comments. |
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Removed the modification of ozone-default.xml as I switched to use the annotation based configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tmp directory might be emptied by admins and others without knowledge of this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK it shouldn't be a problem. This is a socket which is created by the process to listen on and even if it's deleted the socket will work well (until the close).
But I modified it to /var/lib/csi.sock. It can be more meaningful (and maybe more safe If my previous statement is wrong ;-) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we use the new config annotations instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. I uploaded a new version with config annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update this with @config annotations?
|
@elek thanks for the patch. LGTM, added few comments. Seems this needs a rebase as well. |
|
💔 -1 overall
This message was automatically generated. |
|
+1, @elek Can you please fix the checkstyle issues while committing ? Thanks |
|
Thanks the review @anuengineer I am merging it right now (checkstyle issues are fixed). And we can continue the work:
|
|
💔 -1 overall
This message was automatically generated. |
…vel API Author: Prateek Maheshwari <[email protected]> Reviewers: Yi Pan <[email protected]>, Jagadish Venkataraman <[email protected]> Closes apache#693 from prateekm/intermediate-stream-serde
See: https://issues.apache.org/jira/browse/HDDS-1382 for more details..