Navigation Menu

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

opensearchutil: add missing bulk indexer response item fields #90

Merged
merged 1 commit into from Feb 11, 2022

Conversation

igungor
Copy link
Contributor

@igungor igungor commented Jan 26, 2022

Description

If a bulk request contains any script error, Opensearch server points to
what is wrong with the request. This information is valuable to those
debug what is going on under the hood.

This PR adds missing bulk indexer response item fields.

Reproducer script

#!/bin/bash

INDEX="test"
OSURL="http://localhost:9200"


curl -X POST "$OSURL/$INDEX/_bulk?pretty&refresh=true" -H 'Content-Type: application/json' -d '
{"update":{"_id":"1","_index":"test"}}
{"script":{"source":"ctx._source.x = params.x","params":{"x":2}},"upsert":{"x":"1"}}
'

# null pointer exception: ctx._source.y 
curl -X POST "$OSURL/$INDEX/_bulk?pretty&refresh=true" -H 'Content-Type: application/json' -d '
{"update":{"_id":"1","_index":"test"}}
{"script":{"source":"if (ctx._source.y < params.x) {ctx._source.y = params.x}","params":{"x": 2}},"upsert":{"x":"1"}}
'

Opensearch response

{
  "took" : 287,
  "errors" : false,
  "items" : [
    {
      "update" : {
        "_index" : "test",
        "_type" : "_doc",
        "_id" : "1",
        "_version" : 1,
        "result" : "created",
        "forced_refresh" : true,
        "_shards" : {
          "total" : 2,
          "successful" : 2,
          "failed" : 0
        },
        "_seq_no" : 0,
        "_primary_term" : 1,
        "status" : 201
      }
    }
  ]
}
{
  "took" : 3,
  "errors" : true,
  "items" : [
    {
      "update" : {
        "_index" : "test",
        "_type" : "_doc",
        "_id" : "1",
        "status" : 400,
        "error" : {
          "type" : "illegal_argument_exception",
          "reason" : "failed to execute script",
          "caused_by" : {
            "type" : "script_exception",
            "reason" : "runtime error",
            "script_stack" : [
              "if (ctx._source.y < params.x) {",
              "                          ^---- HERE"
            ],
            "script" : "if (ctx._source.y < params.x) {ctx._source.y = params.x}",
            "lang" : "painless",
            "position" : {
              "offset" : 26,
              "start" : 0,
              "end" : 31
            },
            "caused_by" : {
              "type" : "null_pointer_exception",
              "reason" : null
            }
          }
        }
      }
    }
  ]
}

Check List

  • 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.

@igungor igungor requested review from svencowart and a team as code owners January 26, 2022 11:34
@VijayanB
Copy link
Member

VijayanB commented Jan 27, 2022

@igungor can you amend your first commit instead of empty second commit to keep this PR simple.

git commit --amend  --signoff 

Comment on lines 153 to 166
Type string `json:"type"`
Reason string `json:"reason"`
ScriptStack []string `json:"script_stack"`
Script string `json:"script"`
Lang string `json:"lang"`
Position struct {
Offset int `json:"offset"`
Start int `json:"start"`
End int `json:"end"`
} `json:"position"`
Cause struct {
Type string `json:"type"`
Reason string `json:"reason"`
} `json:"caused_by"`
Copy link
Member

Choose a reason for hiding this comment

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

Are all this fields are non-optional if error exists?

Copy link
Contributor Author

@igungor igungor Jan 28, 2022

Choose a reason for hiding this comment

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

For the case I shared, yes they're non-optional. I'm not sure about other errors cases.

Copy link
Member

Choose a reason for hiding this comment

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

If we are not sure, we should make it optional too so that it will not break. Can you make those extra variables as pointer and add omitempty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@igungor
Copy link
Contributor Author

igungor commented Jan 28, 2022

@igungor can you amend your first commit instead of empty second commit to keep this PR simple.

git commit --amend  --signoff 

Done.

@igungor
Copy link
Contributor Author

igungor commented Feb 2, 2022

PTAL

VijayanB
VijayanB previously approved these changes Feb 9, 2022
@VijayanB
Copy link
Member

VijayanB commented Feb 9, 2022

Looks good. Can you merge it into one commit with sign off or amend for last commit?

If a bulk request contains any script error, Opensearch server points
what is wrong with the request. This information is valuable to those
debugging what is wrong with the scripted request.

Signed-off-by: İbrahim Güngör <igungor@gmail.com>
@igungor
Copy link
Contributor Author

igungor commented Feb 9, 2022

Looks good. Can you merge it into one commit with sign off or amend for last commit?

Done.

Copy link
Member

@vamshin vamshin left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

@VijayanB VijayanB merged commit 56d95cc into opensearch-project:main Feb 11, 2022
@igungor igungor deleted the better-bulk-errors branch February 14, 2022 09:37
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.

None yet

4 participants