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

ENH: support pipeline extensions in pipeline config #3299

Merged
merged 30 commits into from
Sep 15, 2023

Conversation

chenqi0805
Copy link
Collaborator

Description

This PR adds support of pipeline_extensions in pipelines configuration file with the following restriction:

  1. when duplicate extensions are configured in data-prepper-config and pipeline-config, the definition in pipeline_extensions wins
  2. pipeline-config is restricted to a single file when configured with pipeline_extensions.

Issues Resolved

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Unit Test Results

  1 432 files  +   273    1 432 suites  +273   43m 27s ⏱️ + 14m 33s
  5 967 tests +     56    5 966 ✔️ +     57  1 💤 ±0  0  - 1 
11 638 runs  +1 947  11 636 ✔️ +1 948  2 💤 ±0  0  - 1 

Results for commit 4631943. ± Comparison against base commit a911c23.

♻️ This comment has been updated with latest results.

Signed-off-by: George Chen <[email protected]>
@dlvenable
Copy link
Member

when duplicate extensions are configured in data-prepper-config and pipeline-config, the definition in pipeline_extensions wins

I would not say this is a restriction. This is the design. Using this here is stating that we want it to be the override.

@@ -28,6 +30,11 @@ public class PipelinesDataFlowModel {
@JsonInclude(JsonInclude.Include.NON_NULL)
private DataPrepperVersion version;

@JsonProperty("pipeline_extensions")
Copy link
Member

Choose a reason for hiding this comment

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

In #2590 this is proposed as pipeline_configurations. I tend to think that we should keep this original name.

There are a couple reasons:

  1. Extensions are intended for extending Data Prepper. Thus, extensions may need to run before parsing a pipeline configuration.
  2. The available options in pipeline_configurations should be somewhat different from the extensions. Not everything in extensions should be configurable in the pipeline.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will rename this part

private final Map<String, Object> extensionMap;

@Inject
public ExtensionPluginConfigurationResolver(final DataPrepperConfiguration dataPrepperConfiguration,
Copy link
Member

Choose a reason for hiding this comment

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

As noted in another comment, we should not tie extensions configurations directly with pipeline configurations. Can we decouple these somewhat?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would you mind if I do it in the other PR: #3340. There has been some tweaks on the interface

Copy link
Member

Choose a reason for hiding this comment

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

That seems reasonable.

@chenqi0805 chenqi0805 merged commit b2b3249 into main Sep 15, 2023
28 checks passed
@chenqi0805 chenqi0805 deleted the enh/support-pipeline-extensions-in-pipeline-config branch September 15, 2023 00:55
asifsmohammed pushed a commit to asifsmohammed/data-prepper that referenced this pull request Sep 27, 2023
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.

2 participants