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

Add missing Response infos to Bulk/ResponseSet (#1737) #1776

Merged

Conversation

vinzlj
Copy link
Contributor

@vinzlj vinzlj commented Apr 30, 2020

I'm using the Bulk API and the response transfer info & status code get erased by the method _processResponse.

Here's the Response I get from the Client:

Elastica\Response {
  #_queryTime: 0.00088000297546387
  #_responseString: ""
  #_transferInfo: array:26 [
    "url" => "http://127.0.0.1:9200/_bulk?filter_path=-took%2C-items"
    "content_type" => null
    "http_code" => 413
    "header_size" => 60
    "request_size" => 201
    "filetime" => -1
    "ssl_verify_result" => 0
    "redirect_count" => 0
    "total_time" => 0.000824
    "namelookup_time" => 5.3E-5
    "connect_time" => 0.000235
    "pretransfer_time" => 0.000276
    "size_upload" => 0.0
    "size_download" => 0.0
    "speed_download" => 0.0
    "speed_upload" => 0.0
    "download_content_length" => 0.0
    "upload_content_length" => 3462417.0
    "starttransfer_time" => 0.000815
    "redirect_time" => 0.0
    "redirect_url" => ""
    "primary_ip" => "127.0.0.1"
    "certinfo" => []
    "primary_port" => 9200
    "local_ip" => "127.0.0.1"
    "local_port" => 55295
  ]
  #_response: []
  #_status: 413
  #_jsonBigintConversion: false
}

And the ResponseSet I get from the Bulk->send()->_processResponse():

Elastica\Bulk\ResponseSet {
  #_bulkResponses: []
  #_position: 0
  #_queryTime: null
  #_responseString: ""
  #_transferInfo: []
  #_response: []
  #_status: null
  #_jsonBigintConversion: false
}

We're losing all the _transferInfo data along with the status which in my case is what I'd really need to prevent my code from running on error.

Here's the ResponseSet from the changes I made:

Elastica\Bulk\ResponseSet {
  #_bulkResponses: []
  #_position: 0
  #_queryTime: 0.0019330978393555
  #_responseString: ""
  #_transferInfo: array:26 [
    "url" => "http://127.0.0.1:9200/_bulk"
    "content_type" => null
    "http_code" => 413
    "header_size" => 60
    "request_size" => 174
    "filetime" => -1
    "ssl_verify_result" => 0
    "redirect_count" => 0
    "total_time" => 0.001874
    "namelookup_time" => 5.0E-5
    "connect_time" => 0.000387
    "pretransfer_time" => 0.000426
    "size_upload" => 0.0
    "size_download" => 0.0
    "speed_download" => 0.0
    "speed_upload" => 0.0
    "download_content_length" => 0.0
    "upload_content_length" => 3743468.0
    "starttransfer_time" => 0.001863
    "redirect_time" => 0.0
    "redirect_url" => ""
    "primary_ip" => "127.0.0.1"
    "certinfo" => []
    "primary_port" => 9200
    "local_ip" => "127.0.0.1"
    "local_port" => 56270
  ]
  #_response: []
  #_status: 413
  #_jsonBigintConversion: false
}

I opened a PR on 6.x (#1775), but I just read PR should be on master so here it is.

Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the contribution. Code LGTM. Could you add a changelog entry?

@vinzlj vinzlj force-pushed the fix/add-missing-fields-to-response-set-master branch from e26e120 to d89dafe Compare May 6, 2020 15:31
@vinzlj
Copy link
Contributor Author

vinzlj commented May 6, 2020

Thanks, I added it to the Unreleased/Changed section, is that ok?

@vinzlj vinzlj force-pushed the fix/add-missing-fields-to-response-set-master branch from d89dafe to e288b7a Compare May 6, 2020 15:36
@vinzlj
Copy link
Contributor Author

vinzlj commented May 6, 2020

Also I've got a unit test failing on ES 7.3:

1) Elastica\Test\MappingTest::testDynamicTemplate
Failed asserting that an array has the key 'properties'.

It doesn't seem to be related to my code, and the test doesn't fail when I run it locally.

Is that a known randomly failing test?

@vinzlj vinzlj force-pushed the fix/add-missing-fields-to-response-set-master branch 2 times, most recently from ffce3c9 to d3dd029 Compare May 6, 2020 16:11
@vinzlj vinzlj force-pushed the fix/add-missing-fields-to-response-set-master branch from d3dd029 to e4ec3b1 Compare May 6, 2020 18:19
@ruflin ruflin merged commit a65de6b into ruflin:master May 7, 2020
@ruflin
Copy link
Owner

ruflin commented May 7, 2020

Merged as CI was green now. Thanks for the contribution!

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

2 participants