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

Convert Processor enhancement (description, snippet) #35280

Merged
merged 2 commits into from
Nov 7, 2018

Conversation

insukcho
Copy link
Contributor

@insukcho insukcho commented Nov 6, 2018

Sometimes, the users are confused about they can use Convert Processor for changing existing field's type to other types even the existing one is already ingested. This confusion is from the first line of description.
Also, code snippet has not complete syntax, so it is misleading the right usage.

Sometimes, the users are confused about they can use `Convert Processor` for changing existing field's type to other types even the existing one is already ingested. This confusion is from the first line of description.
Also, code snippet has not complete syntax, so it is misleading the right usage.
@cbuescher cbuescher added >docs General docs changes :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP labels Nov 6, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

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.

Hi @insukcho, I left some questions. I agree that if you see confusion with some people around this documentation we should try to improve it, but I wonder if the proposed changes might not provoke more questions. I left some comments along those lines.

docs/reference/ingest/ingest-node.asciidoc Outdated Show resolved Hide resolved
docs/reference/ingest/ingest-node.asciidoc Outdated Show resolved Hide resolved
"field" : "url.port",
"type": "integer"
}
"description": "for student index converting from string id field to integer one ",
Copy link
Member

Choose a reason for hiding this comment

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

Again, this raises the question where or what the student index is. I would keep it more general. What do you think is missing or ambigouous from the previous example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, how about describe pipeline for more general purpose?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could decribe what the processor does. In this case it "converts the content of the id field to an integer". By callind the field e.g. "id_string" you could also make it clearer that the original field is suppoesd to not be an integer but a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree w/ describe more clear description like converts the content of the id field to an integer. But for using "id_string" maybe not a good idea because it was string, but it will be changed to integer after ingestion. So, it can make more confusion.

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.

@insukcho I left some additional suggestions.

@cbuescher cbuescher self-assigned this Nov 7, 2018
@insukcho
Copy link
Contributor Author

insukcho commented Nov 7, 2018

@cbuescher Thanks for your comments.

I make new commit which is following your suggestion except id_string idea. Please check my last comments above and check second commit~!

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.

@insukcho thanks for the changes, LGTM. I will merge to master and the current 6.x branches

@cbuescher
Copy link
Member

@insukcho latest CI failures are unrelated to your change, I will build the docs once again locally and merge if that is successful.

@cbuescher cbuescher merged commit e572a21 into master Nov 7, 2018
cbuescher pushed a commit that referenced this pull request Nov 7, 2018
Sometimes users are confused about whether they can use the Convert Processor
for changing an existing fields type to other types even if the existing one is already
ingested. This confusion is from the first line of description. Changing this and also
adding a some detail to the code snippet.
cbuescher pushed a commit that referenced this pull request Nov 7, 2018
Sometimes users are confused about whether they can use the Convert Processor
for changing an existing fields type to other types even if the existing one is already
ingested. This confusion is from the first line of description. Changing this and also
adding a some detail to the code snippet.
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Nov 8, 2018
* master: (24 commits)
  Replicate index settings to followers (elastic#35089)
  Rename RealmConfig.globalSettings() to settings() (elastic#35330)
  [TEST] Cleanup FileUserPasswdStoreTests (elastic#35329)
  Scripting: Add back lookup vars in score script (elastic#34833)
  watcher: Fix integration tests to ensure correct start/stop of Watcher (elastic#35271)
  Remove ALL shard check in CheckShrinkReadyStep (elastic#35346)
  Use soft-deleted docs to resolve strategy for engine operation (elastic#35230)
  [ILM] Check shard and relocation status in AllocationRoutedStep (elastic#35316)
  Ignore date ranges containing 'now' when pre-processing a percolator query (elastic#35160)
  Add a frozen engine implementation (elastic#34357)
  Put a fake allocation id on allocate stale primary command (elastic#34140)
  [CCR] Enforce auto follow pattern name restrictions (elastic#35197)
  [ILM] rolling upgrade tests (elastic#35328)
  [ML] Add Missing data checking class (elastic#35310)
  Apply `ignore_throttled` also to concrete indices (elastic#35335)
  Make version field names more meaningful  (elastic#35334)
  [CCR] Added HLRC support for pause follow API (elastic#35216)
  [Docs] Improve Convert Processor description (elastic#35280)
  [Painless] Removes extraneous compile method (elastic#35323)
  [CCR] Fail with a better error if leader index is red (elastic#35298)
  ...
@colings86 colings86 added v6.5.0 and removed v6.5.1 labels Nov 9, 2018
pgomulka pushed a commit to pgomulka/elasticsearch that referenced this pull request Nov 13, 2018
Sometimes users are confused about whether they can use the Convert Processor
for changing an existing fields type to other types even if the existing one is already
ingested. This confusion is from the first line of description. Changing this and also
adding a some detail to the code snippet.
@insukcho insukcho deleted the insukcho-patch-1 branch November 19, 2018 03:47
@insukcho
Copy link
Contributor Author

Thank you for your help, @cbuescher !!

@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >docs General docs changes v6.5.0 v6.6.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants