Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding fail-fast feature to fast-serde #519

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

krisso-rtb
Copy link
Contributor

@krisso-rtb krisso-rtb commented Oct 9, 2023

This PR aims to add "fail-fast" feature to the fastserde.

Normally when we have datum writer:

DatumWriter<T> datumWriter = new FastSpecificDatumWriter<>(schema, specificData);

and it's used in a loop as:

datumWriter.write(record, binaryEncoder);

the datumWriter under the hood uses standard avro writer until fast-serializer is generated on the background-thread.

Currently if such generation fails due to any reason we have just one log entry and datumWriter still uses standard avro writer.
We found this approach is sometimes too gentle because it hides an issue for a long time. It's easy to miss one WARN log-entry.

That's why we think "fail-fast" approach would be a nice feature. It can be turned on by setting one of system properties

  • avro.fast.serde.failfast
  • avro.fast.serde.failfast.supplier

By default it's disabled so that it's backward-compatible.


Commits:

  1. Code cleanup
  • private Optional<String> compileClassPath; changed to private String compileClassPath;
  • Resolving compileClassPath extracted to helper method (will be re-used for the new system property)
  • other minor things
  1. bugfixes
  • getFastGenericDeserializerAsync no longer generated StackOverflow (shame on me!)
  • Using AvroCompatibilityHelper in fallback-serializers to fix an issue with missing c-tor in Avro 1.4.
  1. Generation errors logged on ERROR level.
  • We can discuss here - I'm fine with dropping this commit if you think it should stay as WARN.
  1. [fastserde] Adding fail-fast feature.
  • The main reason why this PR was created.

Name "fail-fast" is a kind of industry standard however in this case it may cause confusion with "fast-serde" - we can use another name for this feature (fail-early / force-fastserde / etc.) or give it opposite meaning (e.g. ignore-generation-errors).

@codecov-commenter
Copy link

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d661ed3) 45.77% compared to head (392f5a9) 45.77%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #519      +/-   ##
============================================
- Coverage     45.77%   45.77%   -0.01%     
+ Complexity     4439     4436       -3     
============================================
  Files           403      403              
  Lines         28070    28070              
  Branches       4622     4622              
============================================
- Hits          12850    12848       -2     
  Misses        13664    13664              
- Partials       1556     1558       +2     

see 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@krisso-rtb
Copy link
Contributor Author

Hi @FelixGV @gaojieliu
Did you have a chance to look into this?
Just a kind reminder :)

Copy link
Collaborator

@gaojieliu gaojieliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for the change!

@gaojieliu gaojieliu merged commit ab40879 into linkedin:master Oct 19, 2023
2 checks passed
@krisso-rtb krisso-rtb deleted the fastserde-fail-fast-feature branch October 19, 2023 15:48
@gaojieliu
Copy link
Collaborator

@krisso-rtb
New release:
https://github.com/linkedin/avro-util/releases/tag/0.3.20

@krisso-rtb
Copy link
Contributor Author

That's awesome!
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants