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

Changed http code on create index API with bad input raising NotXContentException to 400 #4773

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

ayushKataria
Copy link
Contributor

@ayushKataria ayushKataria commented Oct 13, 2022

Description

[Describe what this change achieves]
Changed http code on create index API with bad input raising NotXContentException to 400

Issues Resolved

[List any issues this PR will resolve]
#2756

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
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@ayushKataria ayushKataria requested review from a team and reta as code owners October 13, 2022 14:36
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2022

Codecov Report

Merging #4773 (ba6c39f) into main (bdb8efe) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #4773      +/-   ##
============================================
+ Coverage     70.86%   70.88%   +0.02%     
- Complexity    58068    58081      +13     
============================================
  Files          4710     4710              
  Lines        277508   277510       +2     
  Branches      40164    40165       +1     
============================================
+ Hits         196661   196725      +64     
+ Misses        64663    64630      -33     
+ Partials      16184    16155      -29     
Impacted Files Coverage Δ
...src/main/java/org/opensearch/ExceptionsHelper.java 83.87% <100.00%> (+3.47%) ⬆️
.../action/admin/indices/flush/ShardFlushRequest.java 50.00% <0.00%> (-50.00%) ⬇️
...h/aggregations/bucket/terms/ParsedDoubleTerms.java 39.13% <0.00%> (-47.83%) ⬇️
...pensearch/indices/breaker/CircuitBreakerStats.java 27.77% <0.00%> (-41.67%) ⬇️
.../admin/cluster/reroute/ClusterRerouteResponse.java 60.00% <0.00%> (-40.00%) ⬇️
...h/action/ingest/SimulateDocumentVerboseResult.java 60.71% <0.00%> (-39.29%) ⬇️
...luster/routing/allocation/RoutingExplanations.java 62.06% <0.00%> (-37.94%) ⬇️
...java/org/opensearch/threadpool/ThreadPoolInfo.java 56.25% <0.00%> (-37.50%) ⬇️
...pensearch/action/ingest/DeletePipelineRequest.java 31.25% <0.00%> (-37.50%) ⬇️
...earch/action/admin/indices/flush/FlushRequest.java 52.17% <0.00%> (-34.79%) ⬇️
... and 489 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This needs a test, please.

CHANGELOG.md Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@ayushKataria
Copy link
Contributor Author

@dblock Added the test as suggested. Not sure what the tests failing are. Can you help with this?

CHANGELOG.md Outdated Show resolved Hide resolved
@dblock
Copy link
Member

dblock commented Oct 21, 2022

I wrote a quick guide to how to deal with flakey tests in #4868 (bottom of dev guide) - care to try it for me?

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@mch2 mch2 left a comment

Choose a reason for hiding this comment

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

Thanks for taking this @ayushKataria.

@mch2
Copy link
Member

mch2 commented Oct 21, 2022

Retriggered check - unrelated failure.

REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.index.shard.SegmentReplicationIndexShardTests.testReplicaReceivesLowerGeneration" -Dtests.seed=25A1BC6ABB78D54B -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=hu -Dtests.timezone=Australia/Darwin -Druntime.java=17

org.opensearch.index.shard.SegmentReplicationIndexShardTests > testReplicaReceivesLowerGeneration FAILED
    java.lang.AssertionError
        at __randomizedtesting.SeedInfo.seed([25A1BC6ABB78D54B:AACF30236089C3AF]:0)
        at org.junit.Assert.fail(Assert.java:87)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.junit.Assert.assertTrue(Assert.java:53)
        at org.opensearch.index.shard.SegmentReplicationIndexShardTests.assertEqualCommittedSegments(SegmentReplicationIndexShardTests.java:717)
        at org.opensearch.index.shard.SegmentReplicationIndexShardTests.testReplicaReceivesLowerGeneration(SegmentReplicationIndexShardTests.java:306)

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

Gradle Check (Jenkins) Run Completed with:

@dblock
Copy link
Member

dblock commented Nov 7, 2022

@ayushKataria rebase this? lets see if the tests are still flaky, then just work through them

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@ayushKataria
Copy link
Contributor Author

ayushKataria commented Nov 14, 2022

@dblock I was seeing some build errors earlier, but when I synced this today it has gone through successfully.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

See below for CHANGELOG:

CHANGELOG.md Show resolved Hide resolved
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dblock
Copy link
Member

dblock commented Nov 15, 2022

DCO and precommit are failing :( back to you @ayushKataria lmk if you need help (I recommend you squash these commits on your local and sign the 1 commit left with git commit -s).

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dblock
Copy link
Member

dblock commented Nov 21, 2022

@ayushKataria do you need help with DCO or the gradle checks above?

@ayushKataria
Copy link
Contributor Author

ayushKataria commented Nov 22, 2022

@dblock Yes, I would appreciate some help with this. What happened is that for the change in CHANGELOG, I was travelling so I pushed from a different machine but missed the sign off on that. I tried squashing the commits, but since I have merged from upstream, it says "has 2 parents", so, I am not able to squash and push with a single signed off commit. What would you suggest I should do now?

For the gradle check, I am not sure what the issue is. It seems to be failing at a build stage and not a test failure. This is the task it seems to be failing at

Execution failed for task ':distribution:bwc:staged:buildBwcLinuxTar'.
> Building 2.4.0 didn't generate expected file /var/jenkins/workspace/gradle-check/search/distribution/bwc/staged/build/bwc/checkout-2.4/distribution/archives/linux-tar/build/distributions/opensearch-min-2.4.0-SNAPSHOT-linux-x64.tar.gz

@dblock
Copy link
Member

dblock commented Nov 22, 2022

I think the easiest is to do a subset of https://code.dblock.org/2015/08/31/getting-out-of-your-first-git-mess.html, ie. create a new clean branch, merge your changes onto it, then force push that clean slate to the xcontentstatuscode branch on GitHub to update this PR.

git checkout main 
git pull upstream main 
git checkout -b xcontentstatuscode-squashed 
git merge xcontentstatuscode --squash
# examine the diff, ensure it's all nice and clean, resolve any conflicts, edit CHANGELOG
git add .
git commit -s -m "...."
git push origin xcontentstatuscode-squashed:xcontentstatuscode -f

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@ayushKataria
Copy link
Contributor Author

ayushKataria commented Nov 23, 2022

@dblock Thanks, it's gone through now. I am going to save that link for future references.

@dblock dblock merged commit 139c2cf into opensearch-project:main Nov 23, 2022
@dblock
Copy link
Member

dblock commented Nov 23, 2022

Thanks for hanging in here with us @ayushKataria! Merged.

@CEHENKLE @andrross We don't backport this one to 2.x or do we? :)

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.

4 participants