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

UpdateByQuery issue #2226

Open
rtwent opened this issue Sep 23, 2024 · 4 comments
Open

UpdateByQuery issue #2226

rtwent opened this issue Sep 23, 2024 · 4 comments

Comments

@rtwent
Copy link

rtwent commented Sep 23, 2024

Given:
ES: 7.17
ruflin/elastica: 7.2.0 (but the issue is present on the dev version too)

Background: Due to architectural issues - we run updateByQuery in loop. Troubles in debugging had appeared.

By the ES documentation the response of UpdateByQuery request is documented. It seems, for now, is impossible to retrieve the message, why request was failed.

Response::HasError() will always return false, because of error property is not documented for _update_by_query. As a result error message will be empty:

Elastica/src/Response.php

Lines 134 to 139 in e2cb89b

public function hasError()
{
$response = $this->getData();
return isset($response['error']);
}

isOk is a way to identify the response status. But still, no error message will be provided.

Example of failed response from elastic:

{
  "took":2,
  "timed_out":false,
  "total":1,
  "updated":0,
  "deleted":0,
  "batches":1,
  "version_conflicts":1,
  "noops":0,
  "retries":{
    "bulk":0,
    "search":0
  },
  "throttled_millis":0,
  "requests_per_second":-1,
  "throttled_until_millis":0,
  "failures":[
    {
      "index":"xxxxx",
      "type":"_doc",
      "id":"xxxxxx",
      "cause":{
        "type":"version_conflict_engine_exception",
        "reason":"[xxxxxx]: version conflict, required seqNo [15668298], primary term [2]. current document has seqNo [15668306] and primary term [2]",
        "index_uuid":"wUFh0yvBRkavh7cCHAxWcg",
        "shard":"1",
        "index":"xxxxx"
      },
      "status":409
    }
  ]
}
@ruflin
Copy link
Owner

ruflin commented Oct 21, 2024

Is my understanding correct that the info you are looking for is something like getFailures()? There is hasFailedShards() but it only returns a bool:

public function hasFailedShards()

@rtwent
Copy link
Author

rtwent commented Oct 22, 2024

Is my understanding correct that the info you are looking for is something like getFailures()?

Yep, something like that.

But to be honest for clients of the library is more convenient way to use generic ::getErrorMessage() than to keep in mind what kind of update is processed:

$response = $this->index->updateByQuery(...);
if ($response->isOk()) {
  $response->getData();
} else {
  $response->getErrorMessage();
}

(This is just my IMO that is not consistent with ES API)

@ruflin
Copy link
Owner

ruflin commented Oct 29, 2024

@rtwent Would you be interested to take a stab at this and open a PR?

@rtwent
Copy link
Author

rtwent commented Nov 1, 2024

It seems that my problem is in using the obsolete version of Your library. I investigated a bit.
We use 7.2.0 and the code below gave us unexpected behavior:

if ($response->hasError()) {
throw new ResponseException($request, $response);
}

since there is no error in the reponse body (only failures) - the response is successfully populated. No exception is thrown.

I forked the project and tried to reproduce such behaviour on 8.* branch. I was not successful. Http transport now is absent and guzzle throws an exception if something is wrong with the status code:

/**
* @group functional
*/
public function testUpdateByQueryFailure(): void
{
    $index = $this->_createIndex();
    $index->addDocument(new Document('1', ['name' => 'ruflin nicolas']),);
    $index->refresh();

    $this->assertEquals(1, $index->search('nicolas')->count());

    $this->expectException(ClientResponseException::class);
    // force updateByQuery failure by changing immutable field
    $index->updateByQuery('*', new Script('ctx._source._id = "marc"'));
}  

I hope I understood everything correctly. Is there a sense to fix it for ^7.2?

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

No branches or pull requests

2 participants