GOBBLIN-1715: Support for Vectorized row batch pooling #3574

rdsr · 2022-09-30T22:44:38Z

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

My PR addresses the following Gobblin JIRA issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-XXX

Description

Here are some details about my PR, including screenshots (if applicable):
As mentioned in the ticket the existing array resizing is buggy for ORC's vectorized row batch. In almost all the cases we should fall back on the 'enlargeFactor' option. In some cases where throughput may be a big concern. We should enable the proposed approach below. We've seen some benefits of using the current approach.

Major GC pauses have reduced
BEFORE
AFTER
Memory is only allocated when needed which when compared to the old approach allocated a lot more memory even when it was not required. [For details please see the ticket]
BEFORE:
AFTER:

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

[x ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

codecov-commenter · 2022-09-30T23:01:17Z

Codecov Report

Merging #3574 (02cf437) into master (a246e23) will decrease coverage by 0.00%.
The diff coverage is 85.96%.

@@             Coverage Diff              @@
##             master    #3574      +/-   ##
============================================
- Coverage     46.87%   46.87%   -0.01%     
- Complexity    10630    10640      +10     
============================================
  Files          2112     2113       +1     
  Lines         82744    82798      +54     
  Branches       9215     9220       +5     
============================================
+ Hits          38784    38808      +24     
- Misses        40404    40429      +25     
- Partials       3556     3561       +5

Impacted Files	Coverage Δ
...rg/apache/gobblin/writer/GobblinBaseOrcWriter.java	`65.45% <60.00%> (-1.54%)`	⬇️
...n/java/org/apache/gobblin/writer/RowBatchPool.java	`91.48% <91.48%> (ø)`
...a/org/apache/gobblin/cluster/GobblinHelixTask.java	`62.36% <0.00%> (-21.51%)`	⬇️
.../gobblin/cluster/GobblinHelixTaskStateTracker.java	`62.50% <0.00%> (-6.25%)`	⬇️
...t/version/TimestampedDatasetStateStoreVersion.java	`27.77% <0.00%> (-5.56%)`	⬇️
.../apache/gobblin/runtime/api/JobExecutionState.java	`79.43% <0.00%> (-0.94%)`	⬇️
.../java/org/apache/gobblin/cluster/SleepingTask.java	`45.45% <0.00%> (+6.06%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

homatthew

Nits but nothing major. Great work!

homatthew · 2022-10-01T00:20:00Z

gobblin-modules/gobblin-orc/src/main/java/org/apache/gobblin/writer/RowBatchPool.java

+
+    static final String ROW_BATCH_EXPIRY_PERIOD = "orc.row.batch.expiry.period.secs";
+    static final int ROW_BATCH_EXPIRY_PERIOD_DEFAULT = 1;
+


Really minor nits about style. Typically we follow a pattern of having a shared prefix variable for orc.row.batch.expiry. or even just orc.row.batch as the prefix. And then we start the default values with the name DEFAULT_

See

gobblin/gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnConfigurationKeys.java

Lines 109 to 110 in 3733d60

public static final String TOKEN_RENEW_INTERVAL_IN_MINUTES = GOBBLIN_YARN_PREFIX + "token.renew.interval.minutes";

public static final Long DEFAULT_TOKEN_RENEW_INTERVAL_IN_MINUTES = Long.MAX_VALUE;

for an example.

Also, maybe add the word pool to these settings since these are specific to the batch pool and not for regular row batch?

gobblin-modules/gobblin-orc/src/main/java/org/apache/gobblin/writer/GobblinBaseOrcWriter.java

gobblin-modules/gobblin-orc/src/main/java/org/apache/gobblin/writer/RowBatchPool.java

gobblin-modules/gobblin-orc/src/main/java/org/apache/gobblin/writer/GobblinBaseOrcWriter.java

homatthew · 2022-10-01T02:12:48Z

gobblin-modules/gobblin-orc/src/main/java/org/apache/gobblin/writer/GobblinBaseOrcWriter.java

@@ -269,6 +280,7 @@ public void commit()
  @Override
  public void write(D record)
      throws IOException {
+    Preconditions.checkState(!closed, "Writer already closed");


When did you see this edge case happen? And does this cause the fork to immediately terminate?

Given there's so much usage and abstract classes which this extends I wanted to be sure that this condition is preserved

homatthew · 2022-10-01T02:15:59Z

gobblin-modules/gobblin-orc/src/main/java/org/apache/gobblin/writer/RowBatchPool.java

+        synchronized (rowBatches) {
+            LinkedList<RowBatchHolder> vals = rowBatches.get(schema);
+            VectorizedRowBatch rowBatch;
+            if (vals == null || vals.size() == 0) {


Dumb question but do we have a preference between apache commons CollectionUtils.isEmpty and a basic null check like this?

I'm not keen on adding more dependencies here if this is a small change

gobblin-modules/gobblin-orc/src/main/java/org/apache/gobblin/writer/RowBatchPool.java

umustafi · 2022-10-03T19:34:44Z

btw service tests are failing checkstyle because you need the apache docstring header at the top of new files

ZihanLi58

+1

* upstream/master: move dataset handler code before cleaning up staging data (apache#3594) [GOBBLIN-1730] Include flow execution id when try to cancel/submit job using SimpleKafkaSpecProducer (apache#3588) [GOBBLIN-1734] make DestinationDatasetHandler work on streaming sources (apache#3592) give option to cancel helix workflow through Delete API (apache#3580) [GOBBLIN-1728] Fix YarnService incorrect container allocation behavior (apache#3586) Support multiple node types in shared flowgraph, fix logs (apache#3590) Search for dummy file in writer directory (apache#3589) Use root cause for checking if exception is transient (apache#3585) [GOBBLIN-1724] Support a shared flowgraph layout in GaaS (apache#3583) [GOBBLIN-1731] Enable HiveMetadataWriter to override table schema lit… (apache#3587) [GOBBLIN-1726] Avro 1.9 upgrade of Gobblin OSS (apache#3581) [GOBBLIN-1725] Fix bugs in gaas warm standby mode (apache#3582) [GOBBLIN-1718] Define DagActionStoreMonitor to listen for kill/resume… (apache#3572) Add log line for committing/retrieving watermarks in streaming (apache#3578) [GOBBLIN-1707] Enhance `IcebergDataset` to detect when files already at dest then proceed with only delta (apache#3575) Ignore AlreadyExistsException in hive writer (apache#3579) Fail GMIP container for known transient exceptions to avoid data loss (apache#3576) GOBBLIN-1715: Support vectorized row batch pooling (apache#3574) [GOBBLIN-1696] Implement file based flowgraph that detects changes to the underlying… (apache#3548) GOBBLIN-1719 Replace moveToTrash with moveToAppropriateTrash for hadoop trash (apache#3573) [GOBBLIN-1703] avoid double quota increase for adhoc flows (apache#3550)

rdsr force-pushed the rbv2 branch from 563095a to 5ecf978 Compare September 30, 2022 22:44

homatthew approved these changes Oct 1, 2022

View reviewed changes

umustafi approved these changes Oct 3, 2022

View reviewed changes

gobblin-modules/gobblin-orc/src/main/java/org/apache/gobblin/writer/RowBatchPool.java Show resolved Hide resolved

GOBBLIN-1715: Support vectorized row batch pooling

02cf437

rdsr force-pushed the rbv2 branch from 5ecf978 to 02cf437 Compare October 7, 2022 19:52

ZihanLi58 approved these changes Oct 11, 2022

View reviewed changes

ZihanLi58 merged commit fafd40b into apache:master Oct 11, 2022

phet pushed a commit to phet/gobblin that referenced this pull request Dec 3, 2022

GOBBLIN-1715: Support vectorized row batch pooling (apache#3574)

62e4a91

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GOBBLIN-1715: Support for Vectorized row batch pooling #3574

GOBBLIN-1715: Support for Vectorized row batch pooling #3574

rdsr commented Sep 30, 2022

codecov-commenter commented Sep 30, 2022 •

edited

Loading

homatthew left a comment

homatthew Oct 1, 2022

homatthew Oct 1, 2022

rdsr Oct 1, 2022

homatthew Oct 1, 2022

rdsr Oct 7, 2022

umustafi commented Oct 3, 2022

ZihanLi58 left a comment


		static final String ROW_BATCH_EXPIRY_PERIOD = "orc.row.batch.expiry.period.secs";
		static final int ROW_BATCH_EXPIRY_PERIOD_DEFAULT = 1;

	public static final String TOKEN_RENEW_INTERVAL_IN_MINUTES = GOBBLIN_YARN_PREFIX + "token.renew.interval.minutes";
	public static final Long DEFAULT_TOKEN_RENEW_INTERVAL_IN_MINUTES = Long.MAX_VALUE;

GOBBLIN-1715: Support for Vectorized row batch pooling #3574

GOBBLIN-1715: Support for Vectorized row batch pooling #3574

Conversation

rdsr commented Sep 30, 2022

JIRA

Description

Tests

Commits

codecov-commenter commented Sep 30, 2022 • edited Loading

Codecov Report

homatthew left a comment

Choose a reason for hiding this comment

homatthew Oct 1, 2022

Choose a reason for hiding this comment

homatthew Oct 1, 2022

Choose a reason for hiding this comment

rdsr Oct 1, 2022

Choose a reason for hiding this comment

homatthew Oct 1, 2022

Choose a reason for hiding this comment

rdsr Oct 7, 2022

Choose a reason for hiding this comment

umustafi commented Oct 3, 2022

ZihanLi58 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 30, 2022 •

edited

Loading