[GitHub] [beam] dspasic commented on a diff in pull request #24841: Eliminate Finalize in Read

[GitBox <gi...@apache.org>]

dspasic commented on code in PR #24841:
URL: https://github.com/apache/beam/pull/24841#discussion_r1059779851


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/Read.java:
##########
@@ -390,13 +409,10 @@ public boolean tryClaim(TimestampedValue<T>[] position) {
       }
 
       @Override
-      protected void finalize() throws Throwable {
+      public void close() throws IOException {

Review Comment:
   > IOException will now bubble up rather than logging, however this adds assumption that the caller of `BoundedSourceAsSDFRestrictionTracker.close()` will catch the Exception so that it does not fail the bundle (currently it satisfies).
   
   Hi @abacn. First of all, happy new year, and thank you for your feedback. 
   
   I agree that the exception propagates upwards, so the client has to handle it. In this case, it is less critical because the close method was added so that we can exclude an existing use case. I also assume that nobody calls the finalize method explicitly and that the call is done implicitly via the GC. Since the finalize method had protected as visibility, the probability of direct access outside the scope is very low, especially since the class BoundedSourceAsSDFRestrictionTracker is private and can exclude an inheritance. However, a residual risk still exists that explicit access to finalize exists, leading to a problem since it is replaced by close in this approach.
   
   > Also note that both GC's finalize() and Beam's tearDown are best effort. The consequence of this change of behavior in real scenario is not clear. To avoid unexpected impact I personally conservative on core codes change that currently work property.
   
   These are also my concerns. This case would have to be investigated more closely under certain conditions to get a clearer picture. This would require experienced people from the user and core developer perspective to accompany this case. At least, that would be my current suggestion. 
   
   The question that arises for me is how we want to move forward with the current task. As already mentioned in the task[0], from my point of view, all finalize methods that the teardown context can replace are done. However, there are 7 more left.  
   
   We can continue here until we have replaced all finalize methods appropriately. On the other hand, I wonder how urgent these adjustments are right now. 
   
   What do you think about this?
   
   [0] https://github.com/apache/beam/issues/24181#issuecomment-1362250796 
   



-- 
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: github-unsubscribe@beam.apache.org

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