hemantk-12 commented on PR #5061: URL: https://github.com/apache/ozone/pull/5061#issuecomment-1638972143
> @hemantk-12 @smengcl @prashantpogde I think this PR was incorrectly merged with some existing problems: > > * Does this call need to go to the running OM, or can the compaction dag be read by the `ozone debug` process from disk state if run on the OM host? The later is how most Ozone debug calls work. Having a network call that generates a file on the server is strange, which brings me to my other concerns. > > * The PR was merged with an [existing security](https://github.com/apache/ozone/pull/5061#discussion_r1263077404) concern not addressed. This definitely needs to be restricted to admins since it could be invoked indefinitely to fill up the OM OS disk. > > * There is no regard for absolute or relative paths in the filename parameter. Either some server side handling or documentation in the help message should either handle this case or explain that the base path is fixed. > > * Help message does not specify that the image is created on the server side. Users may think the command is broken. > > * Response should include which OM generated the file. It will only exist on the OM who is the leader at the time the command was run. > > > If this command can be made to run locally on an OM host without depending on the OM process then a lot of the above concerns should be addressed. Otherwise I think these will need to be fixed, preferably with high priority (especially the security concern). We have enough [tech debt](https://issues.apache.org/jira/browse/HDDS-8097) around Ozone troubleshooting, let's not add more. Thanks for the review and suggestion @errose28 The reason I made it a OM call because I needed current state of DAG. You can argue that we can get that from compaction log but I had slight doubt that it won't give you exact picture as in memory. I think most of the concerns can be resolved if I change OM request to return the DAG as java object and construct the image on client side or use compaction log instead of the way it was done. Should we revert this PR in meanwhile I make the change to address your concern? Or just create new one with fix? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
