sumitagrawl commented on PR #7602: URL: https://github.com/apache/ozone/pull/7602#issuecomment-2565204651
> > ExecutionContext here have just one entry, but in new flow, it’s context for execution which will contains other members as required in future like both index and termIndex. Additional other parameters. It’s problem to change interface for every new parameters. > > * in POC ([HDDS-11418. leader executor framework [prototype] #7406](https://github.com/apache/ozone/pull/7406)) `ExecutionContext` has one additional member: `batchOperation` > * `ExecutionContext` is just a holder for "stuff", with `get`/`set` methods for each member > * `index`/`termIndex` and `batchOperation` are used in unrelated places > * instances of `ExecutionContext` may or may not have these members set > * `RequestContext` is also a holder for "stuff" > * `RequestContext` even has entry for `ExecutionContext` > > So I think `ExecutionContext` is unnecessary and prone to be misused. > > Why not put additional other parameters (`batchOperation` for now) in `RequestContext`? @adoroszlai RequestContext is having wider information which is not required for any execution as can have misuse. Prototype and actual code will have some difference. ExecutionContext: - index (ozone managed index) - ratisIndex (as present for old flow, may not be required once old flow is removed) PostExecutionContext (new class for specific case) - ExecutionContext - batchOperation - ... Current way of passing all parameter via method signature is not a recommended practice, that is the reason we have findbug to restrict parameter count less than six. But we have misuse that by supressing. Recommended practice is to pass via pojo if number of paramenter increases. Another mis-use done due to method restirction is use of Thread-Local. Another mis-use, setting request object and other parameter in Base Class, and making complicated with derivation. ExecutionContext (as pojo) is recommended way to pass parameter if parameter can increase, or have scope of expansion. I think with this, it can provide expansion of feature without impacting existing implementation. So I think above explanation clarify its usecase and better way of coding (as suggested by findbug also). -- 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]
