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.

Sign up for GitHub

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

Jump to bottom

Update PipelineModel to fix serialization #577

Merged
merged 1 commit into from
Nov 12, 2021
Merged

Update PipelineModel to fix serialization #577

merged 1 commit into from
Nov 12, 2021

Conversation

sshivanii
Copy link
Contributor

@sshivanii sshivanii commented Nov 12, 2021 •
edited
Loading

Signed-off-by: Shivani Shukla [email protected]

Description

Fixes serialization in PipelineModel

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

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.

@sshivanii sshivanii requested a review from dlvenable November 12, 2021 18:53
@dlvenable
Copy link
Member

Thanks! This is a good change. I think it might be good to test the serialization and deserialization since that is what caused the issue. But, I'm also happy to approve it as it is and come back to it.

@sshivanii
Copy link
Contributor Author

Thanks! This is a good change. I think it might be good to test the serialization and deserialization since that is what caused the issue. But, I'm also happy to approve it as it is and come back to it.

I'm creating a separate PR for PipelinesDataFlowModel with an object mapper which will test serialization. Do we want to test se/deserialization as part of PipelineModelTest too?

@dlvenable
Copy link
Member

We can combine the tests into one if that works better.

@sshivanii sshivanii requested a review from chenqi0805 November 12, 2021 20:49
chenqi0805
chenqi0805 approved these changes Nov 12, 2021
View reviewed changes
Copy link
Collaborator

@chenqi0805 chenqi0805 left a comment

Choose a reason for hiding this comment

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

changes look good to me. But is this PR just about renaming variables? I did not see how it fixes serialization.

@chenqi0805
Copy link
Collaborator

changes look good to me. But is this PR just about renaming variables? I did not see how it fixes serialization.

Oh. Wait. I see those JsonProperty's. No issue.

@dlvenable
Copy link
Member

It changes the serialization. Prior to this commit, Jackson would serialize these as:

sourcePluginModel:
  http:
prepperPluginModel:
  - grok:
sinkPluginModel:
  - opensearch:

@sshivanii sshivanii merged commit 15c944a into opensearch-project:main Nov 12, 2021
chenqi0805 added a commit to chenqi0805/data-prepper that referenced this pull request Nov 12, 2021
@chenqi0805
Merge branch 'main' into add/GrokLogstashPluginAttributesMapper
385c3bc 
* main:
  Fixes the main branch from a renaming that happened when two related PRs were merged around the same time. (opensearch-project#583)
  Grpc Basic auth for Otel trace Source (opensearch-project#570)
  Fixed a NullPointerException which LogstashVisitor was throwing for attributes without a value. (opensearch-project#582)
  Update PipelineModel to fix serialization (opensearch-project#577)
  Log analytics getting started (opensearch-project#573)
  Support maps which are not present in the YAML, which Jackson appears to be treating differently from explicit nulls. opensearch-project#568 (opensearch-project#579)
  Add support for codeowners to repo (opensearch-project#578)
  Added logstash mapper which maps logstash configuration to pipeline m… (opensearch-project#575)
  Document OpenSearch Sink configuration parameters number_of_shards and number_of_replicas. (opensearch-project#562)
  Added instructions to build and run the Docker image locally. (opensearch-project#564)
  NPE bug fixes in the DefaultLogstashPluginAttributesMapper (opensearch-project#574)
  switch Path.of to Paths.get (opensearch-project#566)
  Fix PrepperState javadoc (opensearch-project#567)

Signed-off-by: qchea <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenqi0805 chenqi0805 chenqi0805 approved these changes

@dlvenable dlvenable dlvenable approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sshivanii @dlvenable @chenqi0805