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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
