Skip to content

Conversation

@andrewbanchich
Copy link
Contributor

fixes spelling and grammatical issues

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@andrewbanchich these are really a couple of great changes. I left some small suggestions and marked a few spots where I felt there might be something missing. Looking forward to the update.

`post` type in the `twitter` index and the `_doc` type in the `blog` index. For more
specific parameters, you can use `query`.

The Reindex API makes no effort handle ID collisions. For such issues, the target index
Copy link
Member

Choose a reason for hiding this comment

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

nit: "to handle"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

If you want a particular set of documents from the twitter index you'll
need to sort. Sorting makes the scroll less efficient but in some contexts
If you want a particular set of documents from the `twitter` index you'll
need to `sort`. Sorting makes the scroll less efficient but in some contexts
Copy link
Member

Choose a reason for hiding this comment

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

nit: If this is not refering to the verb "sort" but to the dsl command in the following example, I would use "you'll need to use sort".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

need to `sort`. Sorting makes the scroll less efficient but in some contexts
it's worth it. If possible, prefer a more selective query to `size` and `sort`.
This will copy 10000 documents from `twitter` into `new_twitter`:
This will copy 10,000 documents from `twitter` into `new_twitter`:
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd leave this as is, the comma is used in many countries (e.g. in Germany and most of the EU) as the decimal separator, so to me personally this reads more like 10 (with a weird extra zero). The existing version might not be as good to read but is less ambiguous I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

// TEST[setup:big_twitter]

Which you also can verify works with:
You can also verify works with:
Copy link
Member

Choose a reason for hiding this comment

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

"it works" or "this works" maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed


You can limit the documents by adding a type to the `source` or by adding a
query. This will only copy ++tweet++'s made by `kimchy` into `new_twitter`:
query. This will only copy ++tweet++s made by `kimchy` into `new_twitter`:
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if you could close #29231 and use this PR for those changes instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All set

Copy link
Member

Choose a reason for hiding this comment

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

Great, could you also remove the special formatting of "tweet" here as suggested in #29231?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot about that. Fixed!

@cbuescher cbuescher self-assigned this Mar 26, 2018
Fixes a few other issues
@andrewbanchich
Copy link
Contributor Author

@cbuescher All set with these changes!

removes special formatting of "tweet"
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@andrewbanchich that for the update, much appreciated. I will kick of a final test run before I merge.

@cbuescher
Copy link
Member

@elasticmachine test this please

@cbuescher cbuescher merged commit 2ba6377 into elastic:6.2 Mar 27, 2018
martijnvg added a commit that referenced this pull request Mar 28, 2018
* es/master: (22 commits)
  Fix building Javadoc JARs on JDK for client JARs (#29274)
  Require JDK 10 to build Elasticsearch (#29174)
  Decouple NamedXContentRegistry from ElasticsearchException (#29253)
  Docs: Update generating test coverage reports (#29255)
  [TEST] Fix issue with HttpInfo passed invalid parameter
  Remove all dependencies from XContentBuilder (#29225)
  Fix sporadic failure in CompositeValuesCollectorQueueTests
  Propagate ignore_unmapped to inner_hits (#29261)
  TEST: Increase timeout for testPrimaryReplicaResyncFailed
  REST client: hosts marked dead for the first time should not be immediately retried (#29230)
  TEST: Use different translog dir for a new engine
  Make SearchStats implement Writeable (#29258)
  [Docs] Spelling and grammar changes to reindex.asciidoc (#29232)
  Do not optimize append-only if seen normal op with higher seqno (#28787)
  [test] packaging: gradle tasks for groovy tests (#29046)
  Prune only gc deletes below local checkpoint (#28790)
  remove testUnassignedShardAndEmptyNodesInRoutingTable
  #28745: remove extra option in the composite rest tests
  Fold EngineDiskUtils into Store, for better lock semantics (#29156)
  Add file permissions checks to precommit task
  ...
martijnvg added a commit that referenced this pull request Mar 28, 2018
* es/6.x:
  Fix building Javadoc JARs on JDK for client JARs (#29274)
  Require JDK 10 to build Elasticsearch (#29174)
  Decouple NamedXContentRegistry from ElasticsearchException (#29253)
  Docs: Update generating test coverage reports (#29255)
  [TEST] Fix issue with HttpInfo passed invalid parameter
  Remove all dependencies from XContentBuilder (#29225)
  Fix sporadic failure in CompositeValuesCollectorQueueTests
  Propagate ignore_unmapped to inner_hits (#29261)
  TEST: Increase timeout for testPrimaryReplicaResyncFailed
  REST client: hosts marked dead for the first time should not be immediately retried (#29230)
  Make SearchStats implement Writeable (#29258)
  [Docs] Spelling and grammar changes to reindex.asciidoc (#29232)
  [test] packaging: gradle tasks for groovy tests (#29046)
  remove testUnassignedShardAndEmptyNodesInRoutingTable
  Add file permissions checks to precommit task
  Remove execute mode bit from source files
  #28745: remove 7.x option in the composite rest tests.
  Optimize the composite aggregation for match_all and range queries (#28745)
  Clarify deprecation warning for auto_generate_phrase_query (#29204)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants