jsedding commented on PR #1371: URL: https://github.com/apache/jackrabbit-oak/pull/1371#issuecomment-2116952625
> [...] i am not sure that the new diff is actually good or still needs some work. @anchela At the risk of explaining things you already know, here I go. Most remaining diffs relate to the metatype, which is informational and used predominantly for the rendering of configuration UIs. The most risky diff in the metatype is IMHO the removal of the Designates > pid field. This seems to be a recurring pattern in this sort of refactoring, and I assume that it defaults to the value of the line above (not sure what it's called). The only change in the declarative services XML is the change of two field from method injection to field injection. I did this, because I suspect that the previous implementation was not 100% correct. I think that during an "update" of a bound service, if the reference is dynamic (which it isn't here), the "bind" method is called before the "unbind" method. I.e. in "unbind" you need to check if the value in the field and the value to be unbound are the same object before you set it to null. Since the bind/unbind methods in question had no other logic than setting the field, I thought it safer to switch them to field injection. TL;DR - With all that said, I am confident that this PR is good to be merged. 🙂 -- 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: dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
