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

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

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


##########
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 does).
   
   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 that currently work property. 



##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/Read.java:
##########
@@ -270,6 +272,8 @@ public void populateDisplayData(DisplayData.Builder builder) {
     private static final Logger LOG = LoggerFactory.getLogger(BoundedSourceAsSDFWrapperFn.class);
     private static final long DEFAULT_DESIRED_BUNDLE_SIZE_BYTES = 64 * (1 << 20);
 
+    private final ArrayDeque<Closeable> restrictionTrackerStack = new ArrayDeque<>(5);

Review Comment:
   Not obvious to me why use a Dequeue of size 5 here.



-- 
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