Skip to content

Allow pass a default value for Event.getField#9115

Open
yaauie wants to merge 2 commits intoelastic:mainfrom
yaauie:feature/issues/8304
Open

Allow pass a default value for Event.getField#9115
yaauie wants to merge 2 commits intoelastic:mainfrom
yaauie:feature/issues/8304

Conversation

@yaauie
Copy link
Copy Markdown
Member

@yaauie yaauie commented Feb 7, 2018

Supersedes: #8315

@yaauie yaauie mentioned this pull request Feb 8, 2018
Closed
andrewvc
andrewvc requested changes Feb 8, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@andrewvc andrewvc 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 mostly! Can we add a test for the ruby variant of this method?

@yaauie
Copy link
Copy Markdown
Member Author

yaauie commented Feb 9, 2018

@andrewvc I've added the requested ruby-api specs; mind taking one more look?

@andrewvc
Copy link
Copy Markdown
Contributor

andrewvc commented Feb 9, 2018

@yaauie looks like there's a failing test now:


23:23:25 org.logstash.EventTest > testGetFieldDefaultValue FAILED
23:23:25     java.lang.AssertionError: expected: java.lang.Long<42> but was: org.jruby.RubyFixnum<42>
23:23:25         at org.junit.Assert.fail(Assert.java:88)
23:23:25         at org.junit.Assert.failNotEquals(Assert.java:834)
23:23:25         at org.junit.Assert.assertEquals(Assert.java:118)
23:23:25         at org.junit.Assert.assertEquals(Assert.java:144)
23:23:25         at org.logstash.EventTest.testGetFieldDefaultValue(EventTest.java:205)

@yaauie
event: minor restructuring of get-field-or-default to save heap churn
781d9ef 
Restructure overloaded methods on `Event` and its jruby-ext counterpart to
provide a single shared implementation of get-field-or-default, which will
reduce heap churn; values that are ruby-typed are no longer deep-converted
to java-types and back to ruby-types, which mirrors pre-existing behaviour
of event get-field before implementation of default-value fallback.

Also adds tests for ruby use of `Event#get(reference, defaultValue)`
@yaauie yaauie force-pushed the feature/issues/8304 branch from 57b970e to 781d9ef Compare February 12, 2018 18:26
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 5, 2025

This pull request does not have a backport label. Could you fix it @yaauie? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
  • backport-8.x is the label to automatically backport to the 8.x branch.

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 5, 2025

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Mar 5, 2025
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 5, 2025

This pull request is now in conflicts. Could you fix it @yaauie? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feature/issues/8304 upstream/feature/issues/8304
git merge upstream/main
git push upstream feature/issues/8304

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewvc andrewvc andrewvc requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

backport-8.x Automated backport to the 8.x branch with mergify status:changes-requested

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yaauie @andrewvc @roaksoax @liketic