Skip to content

Conversation

@alexeykudinkin
Copy link
Contributor

@alexeykudinkin alexeykudinkin commented Nov 11, 2022

Change Logs

In #5416, we added new DisruptorExecutor relying on LMAX Disruptor in lieu of BoundedInMemoryQueue for queueing records.

However following things were missed during review:

  • Missed adding Disruptor as a dep included into the bundles.
  • Missed wiring QueueBasedExecutorFactory into HoodieMergeHelper (as well as others)

This PR rectifies these issues.

Impact

Low

Risk level (write none, low medium or high below)

Low

Documentation Update

N/A

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@alexeykudinkin
Copy link
Contributor Author

@xushiyan PTAL

@alexeykudinkin alexeykudinkin changed the title [MINOR] Fixing Spark bundles (missing Disruptor as dep) [MINOR] Fixing QueueBasedExecutor in Spark bundles (missing Disruptor as dep) Nov 12, 2022
@hudi-bot
Copy link
Collaborator

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@xushiyan xushiyan self-assigned this Nov 13, 2022
@xushiyan xushiyan added the priority:critical Production degraded; pipelines stalled label Nov 13, 2022
<include>org.antlr:stringtemplate</include>
<include>org.apache.parquet:parquet-avro</include>

<include>com.lmax:disruptor</include>
Copy link
Member

Choose a reason for hiding this comment

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

don't we want to shade this lib? it's purely internal right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need to shade it, it's unlikely other engines will be using it

Copy link
Member

Choose a reason for hiding this comment

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

let me add an open question in bundle standards, where we need to put out some guideline for shading

<include>org.antlr:stringtemplate</include>
<include>org.apache.parquet:parquet-avro</include>

<include>com.lmax:disruptor</include>
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@alexeykudinkin alexeykudinkin changed the title [MINOR] Fixing QueueBasedExecutor in Spark bundles (missing Disruptor as dep) [HUDI-5209] Fixing QueueBasedExecutor in Spark bundles (missing Disruptor as dep) Nov 14, 2022
@xushiyan xushiyan merged commit 6b73c81 into apache:master Nov 16, 2022
@xushiyan xushiyan added the dependencies Dependency updates label Nov 16, 2022
Copy link
Contributor

@nsivabalan nsivabalan left a comment

Choose a reason for hiding this comment

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

don't we need it for flink-bundle, kafka-connect-bundle?

@alexeykudinkin
Copy link
Contributor Author

@nsivabalan it's not supported for either of them currently (only wired in HoodieMergeHelper)

fengjian428 pushed a commit to fengjian428/hudi that referenced this pull request Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Dependency updates priority:critical Production degraded; pipelines stalled

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants