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

Fix failures in BulkProcessorIT#testGlobalParametersAndBulkProcessor. #38129

Merged
merged 2 commits into from Feb 5, 2019

Conversation

Projects
None yet
4 participants
@jtibshirani
Copy link
Member

commented Jan 31, 2019

This PR fixes a couple test issues:

  • It narrows an assertWarnings call that was too broad, and wasn't always
    applicable with certain random sequences.
  • Previously, we could send a typeless bulk request containing '_type: 'null'.
    Now we omit the _type key altogether for typeless requests.
Fix failures in BulkProcessorIT#testGlobalParametersAndBulkProcessor.
This PR fixes a couple test issues:
* It narrows an assertWarnings call that was too broad, and wasn't always
  applicable with certain random sequences.
* Previously, we could send a typeless bulk request containing '_type: 'null'.
  Now we omit the _type key altogether for typeless requests.
@elasticmachine

This comment has been minimized.

Copy link

commented Jan 31, 2019

@jtibshirani

This comment has been minimized.

@markharwood

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

These issues originally came to light because @atorok saw this test fail

I can't see the original failure now. Do you have the reproduce line?

It narrows an assertWarnings call that was too broad

I don't see that broader previous assertWarnings in the diff? I can see you've added a new one.
The code that is here in the PR makes sense I'm just trying to reconcile with the stated intent.

@jtibshirani

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2019

Oops, I didn't realize that link would expire. Here was the reproduce line it contained:

./gradlew :client:rest-high-level:integTestRunner \
    -Dtests.seed=A1A91C16AA652749 \
    -Dtests.class=org.elasticsearch.client.BulkProcessorIT \
    -Dtests.method="testGlobalParametersAndBulkProcessor" \
    -Dtests.security.manager=true \
    -Dtests.locale=es-ES \
    -Dtests.timezone=Australia/NSW \
    -Dcompiler.java=11 \
    -Druntime.java=8

The assertWarnings that turned out to be too broad was here: https://github.com/elastic/elasticsearch/pull/38129/files#diff-52c3c5ac9fc17246e52a27ec10e5bfbfL430

@markharwood
Copy link
Contributor

left a comment

LGTM.
There was clearly an issue with serializing null.
I added a comment signalling my unease about the warnings checks we have here - we're not really cleanly testing an API's contract here. We're just squashing some awkward side effects of reusing server-side POJOs on the client - they're setting up warnings in thread locals that have no business appearing in client-side processes but we're having to acknowledge them anyway just to make tests pass. It's ugly (no discredit to you intended) but I guess we have to live with it until we fully divorce HLRC from server-side POJOs.

@@ -448,33 +448,41 @@ private static MultiGetRequest indexDocs(BulkProcessor processor, int numDocs, S
} else {
BytesArray data = bytesBulkRequest(localIndex, localType, i);
processor.add(data, globalIndex, globalType, globalPipeline, null, XContentType.JSON);

if (localType != null) {

This comment has been minimized.

Copy link
@markharwood

markharwood Feb 5, 2019

Contributor

This line troubled me. Bulk requests can pass a type on each request line (localType var here) or pass a default fallback choice which is inherited by any request lines that don't declare a localType (the fallback is called globalType).
So here, I assumed the test's logic should ideally be:

 if (localType !=null || globalType != null) {

These are the conditions under which a user has sent a choice of custom type to the server.
Sadly this doesn't work. It looks like in the server-side BulkRequest that only types expressed at the line level are flagged. It assumes any "global" references to type are already flagged by the main RestBulkAction which we are not exercising in this test. What we are squashing with these assertWarnings checks in BulkProcessorIT are the ThreadContext values normally set up on the server side by the server-side objects like BulkRequest which we are (questionably) re-using in HLRC.
These warning checks are not really the same as simply checking that the response from a _bulk REST interaction has the expected warning if you use happen to use global or local type references in the request. That contract is exercised in BulkRequestWithGlobalParametersIT so I think we're good there.

@jtibshirani

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

Thanks @markharwood for the review. Off the top of my head, it seems like if we had an allowWarnings option, it would be more appropriate here than assertWarnings. This is because we're not trying to test an API contract around warnings, but just allow for the possibility of warnings while testing other functionality.

@jtibshirani jtibshirani merged commit 440d1ed into elastic:master Feb 5, 2019

7 checks passed

CLA Commit author has signed the CLA
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@jtibshirani jtibshirani deleted the jtibshirani:bulk-processor-tests branch Feb 5, 2019

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 5, 2019

Merge branch 'master' into retention-leases-recovery
* master: (23 commits)
  Lift retention lease expiration to index shard (elastic#38380)
  Make Ccr recovery file chunk size configurable (elastic#38370)
  Prevent CCR recovery from missing documents (elastic#38237)
  re-enables awaitsfixed datemath tests (elastic#38376)
  Types removal fix FullClusterRestartIT warnings (elastic#38445)
  Make sure to reject mappings with type _doc when include_type_name is false. (elastic#38270)
  Updates the grok patterns to be consistent with logstash (elastic#27181)
  Ignore type-removal warnings in XPackRestTestHelper (elastic#38431)
  testHlrcFromXContent() should respect assertToXContentEquivalence() (elastic#38232)
  add basic REST test for geohash_grid (elastic#37996)
  Remove DiscoveryPlugin#getDiscoveryTypes (elastic#38414)
  Fix the clock resolution to millis in GetWatchResponseTests (elastic#38405)
  Throw AssertionError when no master (elastic#38432)
  `if_seq_no` and `if_primary_term` parameters aren't wired correctly in REST Client's CRUD API (elastic#38411)
  Enable CronEvalToolTest.testEnsureDateIsShownInRootLocale (elastic#38394)
  Fix failures in BulkProcessorIT#testGlobalParametersAndBulkProcessor. (elastic#38129)
  SQL: Implement CURRENT_DATE (elastic#38175)
  Mute testReadRequestsReturnLatestMappingVersion (elastic#38438)
  [ML] Report index unavailable instead of waiting for lazy node (elastic#38423)
  Update Rollup Caps to allow unknown fields (elastic#38339)
  ...

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

dimitris-athanasiou added a commit to dimitris-athanasiou/elasticsearch that referenced this pull request Feb 12, 2019

Fix failures in BulkProcessorIT#testGlobalParametersAndBulkProcessor. (
…elastic#38129)

This PR fixes a couple test issues:
* It narrows an assertWarnings call that was too broad, and wasn't always
  applicable with certain random sequences.
* Previously, we could send a typeless bulk request containing '_type: 'null'.
  Now we omit the _type key altogether for typeless requests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.