errose28 commented on PR #3360: URL: https://github.com/apache/ozone/pull/3360#issuecomment-1287340191
Hi @JacksonYao287 @sodonnel I still have some concerns about the way this change is implemented. ## No clear problem statement Is the motive to reduce memory usage or RocksDB usage in SCM? If it is memory usage, then we can leave the deleted containers in RocksDB only and not load them into the `ContainerStateMap`. If it is to reduce RocksDB footprint I think this might be a pre-mature optimization as I'm not aware of any RocksDB scalability issues encountered on SCM currently. ## Changing the config key fragments the container delete path Containers deleted before this change is applied remain in SCM forever. Similarly if the config is turned off and on and containers are deleted, they will not be cleaned up when the config is restored to on. I guess this is more of a debugging inconvenience than an error, but will make examining existing clusters confusing none the less. ## No integration with unknown container handling - This change only works when unknown container handling is set to delete. There would need to be some validation that the two configs are set in a way that works with each other, or they would need to be combined to a new config. - Even if the configs are unified, turning the unified config on then off will cause problems. SCM would log reports of unknown containers that were deleted while the config was on. Currently this log only happens when there is a bug in the system, but now there is no way to know whether the config changes or a bug caused the unknown containers. ## Confusion about container delete vs. block delete This is not really a comment on the code but a response to some of the discussion on this PR. The second half of[ this comment](https://github.com/apache/ozone/pull/3360#issuecomment-1117404215) from Stephen and [this comment](https://github.com/apache/ozone/pull/3360#issuecomment-1119204766) from Jackson are definitely valid points, but they concern block deletion, not container deletion. Changes proposed here will not solve these problems. A change in this area will only affect SCM memory usage or rocksDB usage with potential debugging tradeoffs. IMO the best thing to do is: - Do not load the deleted containers into SCM memory. Leave them in RocksDB to be queried if needed. - No config key for this behavior. - Leave unknown container handling as is. - The code path would only be triggered if there is a bug in the system. - In this case the config can be used to resolve the issue if it is determined the containers are not relevant. -- 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]
