jacopoc commented on PR #1166: URL: https://github.com/apache/ofbiz-framework/pull/1166#issuecomment-4398541866
@dixitdeepak , your implementation looks correct and straightforward. Although I haven’t tested it, it should work without causing regressions. That said, while I can see it being useful for specific use cases (for example, when locking based on the value of a single parameter), I have some doubts about whether it is a good fit for more general scenarios. A few more specific remarks: - Specifying the parameter name through the `semaphore-parameter-name` attribute may introduce inconsistencies (for example, the provided name may not match any valid service parameter). Also, it should not be possible to specify an optional parameter. - Restricting this to a single parameter feels somewhat arbitrary and does not seem well suited to more general use cases. For these reasons, instead of `semaphore-parameter-name`, I think it would be better to add an attribute on the service parameter element itself to indicate whether that parameter should be included in the lock. Regarding the data model, I am not sure it is worth the added complexity of introducing a separate entity to store multiple attribute values flexibly. That approach could also make the locking process trickier, less robust, and less efficient. One possible alternative would be to hash all relevant parameter values, append the result to the service name, and keep the existing single-field primary key. In its current form, I still think your patch could be useful for others with similar use cases. However, in my opinion, it may be better to keep it in a Jira ticket or pull request for reference rather than merging it directly into the codebase. -- 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]
