[HUDI-1794] Moved static COMMIT_FORMATTER to thread local variable as SimpleDateFormat is not thread safe. #2819

prashantwason · 2021-04-14T06:34:29Z

Added unit test to ensure new instant time can be generated in multiple threads correctly.

What is the purpose of the pull request

When generating a new instant time in HoodieActiveTimeline, a static instance of SimpleDateFormat is used. This class is not thread safe.

We have a production usecase where multiple HUDI datasets are processed in parallel in different threads of a ThreadPool. Each of these threads creates its own SparkRDDBackedWriteClient and calls startCommit() which generates a new commit time. Because SimpleDateFormat is not thread safe, we get corrupted instant times in several threads.

The solution is to use a thread-specific instance of the SimpleDateFormat for generating new instant times.

Brief change log

Moved static COMMIT_FORMATTER to thread local variable

Verify this pull request

This change added tests and can be verified as follows:

Added test TestHoodieActiveTimeline#testCreateNewInstantTime

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

codecov-io · 2021-04-14T07:20:20Z

Codecov Report

Merging #2819 (eb7d058) into master (65844a8) will decrease coverage by 0.01%.
The diff coverage is 70.00%.

@@             Coverage Diff              @@
##             master    #2819      +/-   ##
============================================
- Coverage     52.55%   52.53%   -0.02%     
- Complexity     3708     3709       +1     
============================================
  Files           484      485       +1     
  Lines         23182    23227      +45     
  Branches       2461     2465       +4     
============================================
+ Hits          12183    12203      +20     
- Misses         9926     9948      +22     
- Partials       1073     1076       +3

Flag	Coverage Δ	Complexity Δ
hudicli	`40.29% <0.00%> (ø)`	`0.00 <0.00> (ø)`
hudiclient	`∅ <ø> (∅)`	`0.00 <ø> (ø)`
hudicommon	`50.67% <100.00%> (-0.01%)`	`0.00 <3.00> (ø)`
hudiflink	`56.54% <ø> (-0.13%)`	`0.00 <ø> (ø)`
hudihadoopmr	`33.33% <ø> (ø)`	`0.00 <ø> (ø)`
hudisparkdatasource	`71.33% <100.00%> (ø)`	`0.00 <0.00> (ø)`
hudisync	`45.70% <ø> (ø)`	`0.00 <ø> (ø)`
huditimelineservice	`64.36% <ø> (ø)`	`0.00 <ø> (ø)`
hudiutilities	`69.79% <ø> (ø)`	`0.00 <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/org/apache/hudi/cli/utils/CommitUtil.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...di/common/table/timeline/HoodieActiveTimeline.java	`67.23% <100.00%> (+0.42%)`	`44.00 <3.00> (+1.00)`
.../spark/sql/hudi/streaming/HoodieStreamSource.scala	`68.67% <100.00%> (ø)`	`21.00 <0.00> (ø)`
.../apache/hudi/sink/partitioner/BucketAssigners.java	`33.33% <0.00%> (-16.67%)`	`2.00% <0.00%> (ø%)`
.../java/org/apache/hudi/common/util/CommitUtils.java	`40.47% <0.00%> (-3.12%)`	`6.00% <0.00%> (ø%)`
...java/org/apache/hudi/sink/StreamWriteFunction.java	`77.77% <0.00%> (-2.74%)`	`22.00% <0.00%> (ø%)`
...g/apache/hudi/common/model/WriteOperationType.java	`50.00% <0.00%> (-1.43%)`	`2.00% <0.00%> (ø%)`
...he/hudi/sink/partitioner/BucketAssignFunction.java	`84.33% <0.00%> (-0.85%)`	`17.00% <0.00%> (ø%)`
.../org/apache/hudi/sink/InstantGenerateOperator.java	`0.00% <0.00%> (ø)`	`0.00% <0.00%> (ø%)`
...n/scala/org/apache/hudi/HoodieSparkSqlWriter.scala	`58.52% <0.00%> (ø)`	`0.00% <0.00%> (ø%)`
... and 3 more

hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java

codecov-commenter · 2021-06-14T19:22:49Z

Codecov Report

Merging #2819 (199e377) into master (769dd2d) will increase coverage by 4.56%.
The diff coverage is 59.25%.

@@             Coverage Diff              @@
##             master    #2819      +/-   ##
============================================
+ Coverage     50.04%   54.60%   +4.56%     
- Complexity     3685     4010     +325     
============================================
  Files           526      530       +4     
  Lines         25466    26026     +560     
  Branches       2886     2992     +106     
============================================
+ Hits          12744    14212    +1468     
+ Misses        11454    10411    -1043     
- Partials       1268     1403     +135

Flag	Coverage Δ
hudicli	`39.95% <0.00%> (ø)`
hudiclient	`∅ <ø> (∅)`
hudicommon	`49.98% <63.63%> (-0.04%)`	⬇️
hudiflink	`60.58% <ø> (ø)`
hudihadoopmr	`51.43% <ø> (ø)`
hudisparkdatasource	`66.53% <100.00%> (ø)`
hudisync	`51.45% <ø> (ø)`
huditimelineservice	`64.36% <ø> (ø)`
hudiutilities	`63.47% <ø> (+54.38%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ain/java/org/apache/hudi/cli/utils/CommitUtil.java	`0.00% <0.00%> (ø)`
.../org/apache/hudi/metadata/HoodieTableMetadata.java	`0.00% <ø> (ø)`
...di/common/table/timeline/HoodieActiveTimeline.java	`65.65% <60.00%> (-1.16%)`	⬇️
...mon/table/timeline/HoodieInstantTimeGenerator.java	`64.70% <64.70%> (ø)`
.../spark/sql/hudi/streaming/HoodieStreamSource.scala	`67.46% <100.00%> (ø)`
...ache/hudi/common/fs/inline/InMemoryFileSystem.java	`79.31% <0.00%> (-10.35%)`	⬇️
...apache/hudi/utilities/sources/AvroKafkaSource.java	`0.00% <0.00%> (ø)`
...callback/kafka/HoodieWriteCommitKafkaCallback.java	`0.00% <0.00%> (ø)`
...ck/kafka/HoodieWriteCommitKafkaCallbackConfig.java	`0.00% <0.00%> (ø)`
...ava/org/apache/hudi/utilities/SqlQueryBuilder.java	`92.50% <0.00%> (ø)`
... and 47 more

prashantwason · 2021-07-02T19:02:01Z

@hudi-bot run azure

prashantwason · 2021-08-07T07:17:44Z

@vinothchandar @ssdong This is ready for a review again.

codope

@prashantwason Few places where COMMIT_FORMATTER is still being used:
StreamerUtil
HoodieSqlUtils
TestTimeTravelQuery

Probably, they were added after this patch. Could you please fix them as well?

nsivabalan · 2021-10-22T05:54:44Z

@prashantwason : can you address the feedback place. We want to get this in for the upcoming release.

…ds to parse and generate Instant timestamps. Replaced SimpleDateFormat with DateTimeFormatter as the former is not thread safe. Added unit test to ensure new instant time can be generated in multiple threads correctly.

prashantwason · 2021-10-27T10:28:41Z

@nsivabalan Updated the PR.

codope · 2021-10-27T16:33:29Z

@prashantwason Can you take a look at the CI failure?

[ERROR] org.apache.hudi.sink.TestStreamWriteOperatorCoordinator.testSyncMetadataTable  Time elapsed: 3.01 s  <<< FAILURE!
java.lang.AssertionError: 

Expected: is "0000000000000"
     but: was "00000000000000"
	at org.apache.hudi.sink.TestStreamWriteOperatorCoordinator.testSyncMetadataTable(TestStreamWriteOperatorCoordinator.java:207)

I think the expectation is that instant should be 14chars yyyyMMddHHmmss. But i'm not sure why it was not failing earlier.

…nst the SOLO_COMMIT_TIMESTAMP for metadata table.

prashantwason · 2021-11-01T19:48:29Z

@codope Fixed the test.

nsivabalan · 2021-11-01T23:18:17Z

@hudi-bot azure run

codope · 2021-11-03T12:57:47Z

@hudi-bot run azure

codope · 2021-11-03T14:47:45Z

@hudi-bot run azure

codope

All comments addressed. CI succeeded. Good to land.

hudi-bot · 2021-11-05T02:52:31Z

CI report:

d057c0d Azure: FAILURE Azure: FAILURE Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

ssdong reviewed Apr 15, 2021

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java Outdated Show resolved Hide resolved

vinothchandar self-assigned this Apr 19, 2021

nsivabalan added the priority:major degraded perf; unable to move forward; potential bugs label May 11, 2021

nsivabalan self-assigned this May 21, 2021

prashantwason force-pushed the pw_fix_commit_formatter branch from eb7d058 to 3deb5e7 Compare June 14, 2021 19:14

prashantwason force-pushed the pw_fix_commit_formatter branch from 3deb5e7 to 199e377 Compare June 15, 2021 05:29

codope requested changes Sep 8, 2021

View reviewed changes

nsivabalan added the priority:blocker label Oct 22, 2021

prashantwason force-pushed the pw_fix_commit_formatter branch from 199e377 to d59d595 Compare October 27, 2021 10:28

Fixed the test by removing the hardcoded timestamp and comparing agai…

d057c0d

…nst the SOLO_COMMIT_TIMESTAMP for metadata table.

codope approved these changes Nov 4, 2021

View reviewed changes

nsivabalan merged commit b7ee341 into apache:master Nov 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-1794] Moved static COMMIT_FORMATTER to thread local variable as SimpleDateFormat is not thread safe. #2819

[HUDI-1794] Moved static COMMIT_FORMATTER to thread local variable as SimpleDateFormat is not thread safe. #2819

prashantwason commented Apr 14, 2021

codecov-io commented Apr 14, 2021 •

edited

Loading

codecov-commenter commented Jun 14, 2021 •

edited

Loading

prashantwason commented Jul 2, 2021

prashantwason commented Aug 7, 2021

codope left a comment

nsivabalan commented Oct 22, 2021

prashantwason commented Oct 27, 2021

codope commented Oct 27, 2021

prashantwason commented Nov 1, 2021

nsivabalan commented Nov 1, 2021

codope commented Nov 3, 2021

codope commented Nov 3, 2021

codope left a comment

hudi-bot commented Nov 5, 2021

[HUDI-1794] Moved static COMMIT_FORMATTER to thread local variable as SimpleDateFormat is not thread safe. #2819

[HUDI-1794] Moved static COMMIT_FORMATTER to thread local variable as SimpleDateFormat is not thread safe. #2819

Conversation

prashantwason commented Apr 14, 2021

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

codecov-io commented Apr 14, 2021 • edited Loading

Codecov Report

codecov-commenter commented Jun 14, 2021 • edited Loading

Codecov Report

prashantwason commented Jul 2, 2021

prashantwason commented Aug 7, 2021

codope left a comment

Choose a reason for hiding this comment

nsivabalan commented Oct 22, 2021

prashantwason commented Oct 27, 2021

codope commented Oct 27, 2021

prashantwason commented Nov 1, 2021

nsivabalan commented Nov 1, 2021

codope commented Nov 3, 2021

codope commented Nov 3, 2021

codope left a comment

Choose a reason for hiding this comment

hudi-bot commented Nov 5, 2021

CI report:

codecov-io commented Apr 14, 2021 •

edited

Loading

codecov-commenter commented Jun 14, 2021 •

edited

Loading