rondagostino commented on PR #13280:
URL: https://github.com/apache/kafka/pull/13280#issuecomment-1500342559

   Thanks for the review, @cmccabe.  No problem with the package change and the 
more precise import controls.
   
   Regarding the wrapper suggestion:
   
   It is true that the wrapper implementation in the PR as it stands right now 
always creates a new object when creating a new persistent collection (either 
by creating an empty/singleton or by adding/removing from an existing one).  
The overhead is low -- object creation generally isn't onerous in time or space 
in general, and the work created by having the additional object will be a very 
low percentage of the work that the underlying persistent collection has to do 
anyway.  So it doesn't concern me very much in that regard, but I do agree we 
should avoid it it is unnecessary.
   
   PCollections and Paguro both support using the `java.util` interfaces 
directly on their implementation classes, but Vavr (or whatever else we might 
decide to abstract away like this) does not.  Such libraries would require the 
use of a wrapper class as is proposed here.  The `PersistentCollectionFactory` 
you proposed would still work -- it would just take wrapper instances and 
delegate to those, which would in turn delegate to the underlying wrapped 
persistent collection.  So there would be 2 levels of indirection instead of 1 
for those libraries (the factory always has to do the downcast and perform the 
delegation, but in the Vavr-like cases it would cast the input to a wrapper and 
delegate to that, which would then in turn do the delegation again to the 
underlying library object).   But delegation is cheap, so I would have no 
problem with that.  In short, being required to introduce a wrapper for some 
libraries doesn't necessarily require us to introduce one for PCollections.  
   
   The last issue I can think of that we should discuss here is type safety.  I 
would prefer to be able to know that something is in fact a persistent 
collection.  Unfortunately there is no way to keep that distinction with your 
proposed factory definition.  This drawback gives me pause.
   
   So I think we have a situation where the current PR introduces a small 
amount of overhead that is not expected to be significant but that we would 
prefer to eliminate of possible.  But the cost is a lack of type safety -- the 
compiler cannot know if a collection is a persistent one or not.
   
   Is this a fair discussion of the tradeoffs?
   
   
   


-- 
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: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to