[GitHub] [beam] TheNeuralBit commented on a diff in pull request #17428: [BEAM-14326] Make sure BigQuery daemon thread doesn't exit suddenly, as this leads to pipeline stuckness

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

TheNeuralBit commented on code in PR #17428:
URL: https://github.com/apache/beam/pull/17428#discussion_r855330571


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiDynamicDestinationsTableRow.java:
##########
@@ -95,7 +96,14 @@ public MessageConverter<T> getMessageConverter(
                       + "using a create disposition of CREATE_IF_NEEDED.");
             }
           }
+        } else {
+          // Make sure we register this schema with the cache, unless there's already a more
+          // up-to-date schema.
+          tableSchema =
+              MoreObjects.firstNonNull(
+                  SCHEMA_CACHE.putSchemaIfAbsent(tableReference, tableSchema), tableSchema);

Review Comment:
   Is this change to fix some exception that was raising in the daemon and crashing it?



##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableSchemaCache.java:
##########
@@ -271,10 +280,11 @@ public void refreshThread() {
       if (timeRemaining.getMillis() > 0) {
         Thread.sleep(timeRemaining.getMillis());
       }
-    } catch (InterruptedException e) {
-      runUnderMonitor(() -> this.stopped = true);
-      return;
-    } catch (IOException e) {
+    } catch (Exception e) {

Review Comment:
   Is it appropriate to swallow _all_ exceptions? Could there be cases where the daemon needs to be monitored and restarted? 



##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableSchemaCache.java:
##########
@@ -271,10 +280,11 @@ public void refreshThread() {
       if (timeRemaining.getMillis() > 0) {
         Thread.sleep(timeRemaining.getMillis());
       }
-    } catch (InterruptedException e) {
-      runUnderMonitor(() -> this.stopped = true);
-      return;
-    } catch (IOException e) {
+    } catch (Exception e) {
+      // Since this is a daemon thread, don't exit until it is explicitly shut down. Exiting early
+      // can cause the
+      // pipeline to stall.
+      LOG.error("Caught exception: " + e);

Review Comment:
   Should this include some message about the context in which the exception was raised? 



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