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 support for documented byte/size units and for micros as a time unit in _cat API #17779

Merged
merged 1 commit into from Apr 29, 2016

Conversation

Projects
None yet
5 participants
@dadoonet
Copy link
Member

commented Apr 15, 2016

We advertise in our documentation that byte units are like kb, mb... But we actually only support the simple notation k or m.
This commit adds support for the documented form and keeps the non documented options to avoid any breaking change.

It also adds support for micros as a time unit in _cat API.

Documentation updated accordingly.

@ywelsch

View changes

core/src/main/java/org/elasticsearch/rest/action/support/RestTable.java Outdated
@@ -339,7 +339,9 @@ private static String renderValue(RestRequest request, Object value) {
if (value instanceof TimeValue) {
TimeValue v = (TimeValue) value;
String resolution = request.param("time");
if ("ms".equals(resolution)) {
if ("micros".equals(resolution)) {

This comment has been minimized.

Copy link
@ywelsch

ywelsch Apr 15, 2016

Contributor

nanos and d (for days) are missing here as well.

This comment has been minimized.

Copy link
@dadoonet

dadoonet Apr 15, 2016

Author Member

Agreed.

@danielmitterdorfer

View changes

docs/reference/cat.asciidoc Outdated

If you want to change the <<size-units,size units>>, use `size` parameter.

If you want to change the <<size-units,byte units>>, use `bytes` parameter.

This comment has been minimized.

Copy link
@danielmitterdorfer

danielmitterdorfer Apr 15, 2016

Member

Should be <<byte-units,byte units>> I guess?

This comment has been minimized.

@danielmitterdorfer

View changes

docs/reference/cat.asciidoc Outdated
@@ -74,7 +74,7 @@ with `bulk.`.
[[numeric-formats]]
=== Numeric formats

Many commands provide a few types of numeric output, either a byte
Many commands provide a few types of numeric output, either a byte or size

This comment has been minimized.

Copy link
@danielmitterdorfer

danielmitterdorfer Apr 15, 2016

Member

I'd prefer: "... either a byte, size or a time value."

This comment has been minimized.

Copy link
@dadoonet

dadoonet Apr 15, 2016

Author Member

Yeah. Thanks.

@ywelsch

View changes

core/src/main/java/org/elasticsearch/rest/action/support/RestTable.java Outdated
@@ -322,15 +322,15 @@ private static String renderValue(RestRequest request, Object value) {
String resolution = request.param("size");
if ("b".equals(resolution)) {
return Long.toString(v.singles());
} else if ("k".equals(resolution)) {
} else if ("k".equals(resolution) || "kb".equals(resolution)) {

This comment has been minimized.

Copy link
@ywelsch

ywelsch Apr 15, 2016

Contributor

afaics kb is not supported for SizeValue. it is "k" or "K"? Same for the other entries.

This comment has been minimized.

Copy link
@dadoonet

dadoonet Apr 15, 2016

Author Member

I see. So I guess I need to add a size section in the guide in addition to size-units we have here: https://www.elastic.co/guide/en/elasticsearch/reference/2.3/common-options.html#size-units
(and rename this size-units to byte-units.

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2016

I updated the PR with a new commit.

  • Remove the support for b as a SizeValue unit. Actually, for numbers, when using raw numbers without unit, there is no text to add/parse after the number. For example, you don't write 10 as 10b.
  • Size values don't have b suffix. So We revert the previous change in renderValue() when it comes to SizeValue and we support option like size= which means that we want to display raw data without unit (singles).
  • Add support for nanos and d for TimeValue
  • Update the documentation accordingly
@@ -175,9 +175,7 @@ public static SizeValue parseSizeValue(String sValue, SizeValue defaultValue) th
}
long singles;
try {
if (sValue.endsWith("b")) {

This comment has been minimized.

Copy link
@ywelsch

ywelsch Apr 15, 2016

Contributor

Can we safely remove this? Isn't this becoming a breaking change?

This comment has been minimized.

Copy link
@ywelsch

ywelsch Apr 15, 2016

Contributor

ok, talked to David about it. We will add a note to breaking change docs.

This comment has been minimized.

Copy link
@ywelsch

ywelsch Apr 15, 2016

Contributor

We looked at the usage places. Currently only used for specifying thread pool sizes. If a user specified that he wants queues of "5b", then it SHOULD break :-)

@@ -320,7 +320,7 @@ private static String renderValue(RestRequest request, Object value) {
if (value instanceof SizeValue) {
SizeValue v = (SizeValue) value;
String resolution = request.param("size");
if ("b".equals(resolution)) {
if ("".equals(resolution)) {

This comment has been minimized.

Copy link
@ywelsch

ywelsch Apr 15, 2016

Contributor

This looks weird. The user would specify "size=" ?

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 15, 2016

Contributor

Maybe "units" or something.

This comment has been minimized.

Copy link
@ywelsch

ywelsch Apr 15, 2016

Contributor

after second thought, ok with it (it's documented)

This comment has been minimized.

Copy link
@ywelsch

ywelsch Apr 15, 2016

Contributor

can we add a test for this?


Whenever the size of data needs to be specified, eg when setting a buffer size
Whenever the byte size of data needs to be specified, eg when setting a buffer size

This comment has been minimized.

Copy link
@ywelsch

ywelsch Apr 15, 2016

Contributor

eg -> e.g.,

@nik9000

View changes

docs/reference/api-conventions.asciidoc Outdated
@@ -378,6 +380,21 @@ supported units are:
`tb`:: Terabytes
`pb`:: Petabytes

[[size-units]]
[float]
=== Data size units

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 15, 2016

Contributor

Call them unit-less quantities. Explain that it is things like "document count" and "queue depth", etc. It is unit-less in that it doesn't have a "unit" like "bytes" or "Hertz" or "meter" or "long tonne".

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 15, 2016

Contributor

I meant this as a conversational question more than a command. I think "data size units" is unclear. I've hear people call them "quantities" before.

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2016

@nik9000 @ywelsch I added 2 new commits.

One for doc
The other one consist of adding a new test (and fix missing options in another one). Note that we are probably missing those global options in every CAT API REST description file.

@@ -19,7 +19,7 @@
"bytes": {
"type": "enum",
"description" : "The unit in which to display byte values",
"options": [ "b", "k", "m", "g" ]
"options": [ "b", "k", "kb", "m", "mb", "g", "gb", "t", "tb", "p", "pb" ]

This comment has been minimized.

Copy link
@ywelsch

ywelsch Apr 15, 2016

Contributor

can you also update cat.fielddata.json, cat.indices.json and cat.recovery.json? (These were returned by grepping "bytes": {)

This comment has been minimized.

Copy link
@dadoonet

dadoonet Apr 15, 2016

Author Member

Sure

@nik9000

View changes

docs/reference/api-conventions.asciidoc Outdated
Unit-less quantities means that they don't have a "unit" like "bytes" or "Hertz" or "meter" or "long tonne".

Whenever the size of data needs to be specified or printed, the value may specify the multiplier like `10m` for
1 000 000 elements. For example a "document count" or "queue depth" could be expressed using this notation.

This comment has been minimized.

Copy link
@nik9000

nik9000 Apr 15, 2016

Contributor

s/1 000 000/10 000 000/ ?

Also, I don't know if we are consistent, but I think we'd use "10,000,000". That is the normal "American" formatting but I think I see it all over our docs.

I might rewrite the second paragraph like
If one of these quantities is large we'll print it out like 10m for 10,000,000 or 7k for 7,000. We'll still print 87 when we mean 87 though. These are the supported multipliers:

Or something like that. I dunno.

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2016

Thanks guys. Updated.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2016

LGTM!

Add support for documented byte/size units and for micros as a time u…
…nit in _cat API

We advertise in our documentation that byte units are like `kb`, `mb`... But we actually only support the simple notation `k` or `m`.
This commit adds support for the documented form and keeps the non documented options to avoid any breaking change.

It also adds support for `micros`, `nanos` and `d` as a time unit in `_cat` API.

Remove the support for `b` as a SizeValue unit. Actually, for numbers, when using raw numbers without unit, there is no text to add/parse after the number. For example, you don't write `10` as `10b`. We support option like `size=` in `_cat` API which means that we want to display raw data without unit (singles).

Documentation updated accordingly.

Add test for the empty size option.

Fix missing TimeValues options for some cat APIs

@dadoonet dadoonet force-pushed the dadoonet:pr/cat-size-time-units branch to 5e1f26c Apr 15, 2016

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

Interesting. So I launched the full tests with gradle check and apparently it now fails in QA with a lot of timeout issues...

Suite: org.elasticsearch.backwards.MultiNodeBackwardsIT
  2> REPRODUCE WITH: gradle :qa:backwards-5.0:integTest -Dtests.seed=CFB477C2B2E2A7B7 -Dtests.class=org.elasticsearch.backwards.MultiNodeBackwardsIT -Dtests.method="test {p0=indices.stats/14_groups/Groups - search metric}" -Des.logger.level=WARN -Dtests.security.manager=true -Dtests.locale=nl -Dtests.timezone=Europe/Belgrade
FAILURE 60.7s | MultiNodeBackwardsIT.test {p0=indices.stats/14_groups/Groups - search metric} <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: expected [2xx] status code but api [index] returned [503 Service Unavailable] [{"error":{"root_cause":[{"type":"unavailable_shards_exception","reason":"[test1][3] primary shard is not active Timeout: [1m], request: [index {[test1][bar][1], source[{\"bar\":\"bar\",\"baz\":\"baz\"}]}]"}],"type":"unavailable_shards_exception","reason":"[test1][3] primary shard is not active Timeout: [1m], request: [index {[test1][bar][1], source[{\"bar\":\"bar\",\"baz\":\"baz\"}]}]"},"status":503}]
   >    at __randomizedtesting.SeedInfo.seed([CFB477C2B2E2A7B7:47E048181C1ECA4F]:0)
   >    at org.elasticsearch.test.rest.section.DoSection.execute(DoSection.java:107)
   >    at org.elasticsearch.test.rest.ESRestTestCase.test(ESRestTestCase.java:387)
   >    at java.lang.Thread.run(Thread.java:745)
  2> REPRODUCE WITH: gradle :qa:backwards-5.0:integTest -Dtests.seed=CFB477C2B2E2A7B7 -Dtests.class=org.elasticsearch.backwards.MultiNodeBackwardsIT -Dtests.method="test {p0=indices.open/20_multiple_indices/All indices}" -Des.logger.level=WARN -Dtests.security.manager=true -Dtests.locale=nl -Dtests.timezone=Europe/Belgrade
FAILURE 30.1s | MultiNodeBackwardsIT.test {p0=indices.open/20_multiple_indices/All indices} <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: expected [2xx] status code but api [cluster.health] returned [408 Request Timeout] [{"cluster_name":"qa_backwards-5.0_integTest","status":"red","timed_out":true,"number_of_nodes":2,"number_of_data_nodes":2,"active_primary_shards":0,"active_shards":0,"relocating_shards":0,"initializing_shards":0,"unassigned_shards":15,"delayed_unassigned_shards":0,"number_of_pending_tasks":0,"number_of_in_flight_fetch":0,"task_max_waiting_in_queue_millis":0,"active_shards_percent_as_number":0.0}]
   >    at __randomizedtesting.SeedInfo.seed([CFB477C2B2E2A7B7:47E048181C1ECA4F]:0)
   >    at org.elasticsearch.test.rest.section.DoSection.execute(DoSection.java:107)
   >    at org.elasticsearch.test.rest.ESRestTestCase.test(ESRestTestCase.java:387)
   >    at java.lang.Thread.run(Thread.java:745)
...

Does this ring a bell to anyone?

I can reproduce it with:

gradle :qa:backwards-5.0:integTest -Dtests.class=org.elasticsearch.backwards.MultiNodeBackwardsIT -Dtests.method="test {p0=indices.stats/14_groups/Groups - search metric}" -Des.logger.level=DEBUG

Here is what I can see in DEBUG:

Suite: org.elasticsearch.backwards.MultiNodeBackwardsIT
  1> [2016-04-16 08:49:58,473][INFO ][org.elasticsearch.test.rest.client] REST client initialized [http://[::1]:64492], elasticsearch version: [5.0.0]
  1> [2016-04-16 08:49:58,474][DEBUG][org.elasticsearch.test.rest] resetting client, response and stash
  1> [2016-04-16 08:49:58,480][INFO ][org.elasticsearch.test.rest.client] REST client initialized [http://[::1]:64492], elasticsearch version: [5.0.0]
  1> [2016-04-16 08:49:58,481][DEBUG][org.elasticsearch.test.rest] resetting client, response and stash
  1> [2016-04-16 08:49:58,481][INFO ][org.elasticsearch.backwards] start setup test [indices.stats/14_groups/Groups - search metric]
  1> [2016-04-16 08:49:58,549][DEBUG][org.elasticsearch.test.rest.client] calling api [index]
  1> [2016-04-16 08:50:59,494][DEBUG][org.elasticsearch.test.rest.client] calling api [indices.delete]
  1> [2016-04-16 08:50:59,518][DEBUG][org.elasticsearch.test.rest.client] calling api [indices.delete_template]
  1> [2016-04-16 08:50:59,520][DEBUG][org.elasticsearch.test.rest.client] calling api [snapshot.delete_repository]
  1> [2016-04-16 08:50:59,523][DEBUG][org.elasticsearch.test.rest.client] calling api [tasks.list]
  1> [2016-04-16 08:50:59,610][INFO ][org.elasticsearch.backwards] Stash dump on failure [{
  1>   "stash" : { }
  1> }]
@ywelsch

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2016

@dadoonet I cannot reproduce the issue. Neither gradle clean check nor gradle :qa:backwards-5.0:integTest -Dtests.class=org.elasticsearch.backwards.MultiNodeBackwardsIT -Dtests.method="test {p0=indices.stats/14_groups/Groups - search metric}" -Des.logger.level=DEBUG fail for me.

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2016

Interesting. So I should may be push the changes as is?

@ywelsch

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2016

try running it again (remember to do the gradle clean), and provide me the seed of the failing test.

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2016

I tried again tonight with gradle clean check from the root directory.

Suite: org.elasticsearch.backwards.MultiNodeBackwardsIT
  2> REPRODUCE WITH: gradle :qa:backwards-5.0:integTest -Dtests.seed=6B2AE5C58B795DD0 -Dtests.class=org.elasticsearch.backwards.MultiNodeBackwardsIT -Dtests.method="test {p0=mget/15_ids/IDs}" -Des.logger.level=WARN -Dtests.security.manager=true -Dtests.locale=ar-SA -Dtests.timezone=Pacific/Ponape
FAILURE 60.2s | MultiNodeBackwardsIT.test {p0=mget/15_ids/IDs} <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: expected [2xx] status code but api [index] returned [503 Service Unavailable] [{"error":{"root_cause":[{"type":"unavailable_shards_exception","reason":"[test_1][3] primary shard is not active Timeout: [1m], request: [index {[test_1][test][1], source[{\"foo\":\"bar\"}]}]"}],"type":"unavailable_shards_exception","reason":"[test_1][3] primary shard is not active Timeout: [1m], request: [index {[test_1][test][1], source[{\"foo\":\"bar\"}]}]"},"status":503}]
   >    at __randomizedtesting.SeedInfo.seed([6B2AE5C58B795DD0:E37EDA1F25853028]:0)
   >    at org.elasticsearch.test.rest.section.DoSection.execute(DoSection.java:107)
   >    at org.elasticsearch.test.rest.ESRestTestCase.test(ESRestTestCase.java:395)
   >    at java.lang.Thread.run(Thread.java:745)

Got this report:

Completed [1/1] in 2408.32s, 81 tests, 45 failures, 2 errors, 1 skipped <<< FAILURES!

Tests with failures (first 25 out of 47):
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=mget/15_ids/IDs}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=indices.get_field_mapping/50_field_wildcards/Get field mapping should work using '_all' for indices and types}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=update/60_refresh/Refresh}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=indices.get_field_mapping/50_field_wildcards/Get field mapping with t* for fields}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=bulk/20_list_of_strings/List of strings}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=termvectors/40_versions/Versions}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=indices.stats/12_level/Level - shards}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=delete/20_internal_version/Internal version}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=percolate/19_nested/Percolate existing docs}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=bulk/10_basic/Array of objects}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=get_source/10_basic/Basic}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=delete/26_external_gte_version/External GTE version}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=indices.get_field_mapping/10_basic/Get field mapping by index only}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=indices.stats/11_metric/Metric - one}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=mget/70_source_filtering/Source filtering -  true/false}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=update/20_doc_upsert/Doc upsert}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=suggest/20_completion/Multiple Completion fields should work}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=indices.get_field_mapping/50_field_wildcards/Get field mapping should work using '*' for indices and types}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=cat.segments/10_basic/Test cat segments output}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=cluster.state/20_filtering/Filtering the cluster state by indices should work in routing table and metadata}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=index/60_refresh/Refresh}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=cluster.state/20_filtering/Filtering the cluster state using _all for indices and metrics should work}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=suggest/20_completion/Simple suggestion array should work}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=indices.stats/10_index/Index - pattern}
  - org.elasticsearch.backwards.MultiNodeBackwardsIT.test {p0=cluster.state/30_expand_wildcards/Test ignore_unavailable parameter}

Slow Tests Summary:
2408.32s | org.elasticsearch.backwards.MultiNodeBackwardsIT

JUnit4 test failed, ant output was:
   [junit4] <JUnit4> says kaixo! Master seed: 6B2AE5C58B795DD0
   [junit4] JVM J0:     0.27 ..  2411.13 =  2410.86s
   [junit4] Execution time total: 40 minutes 11 seconds
   [junit4] Tests summary: 1 suite, 81 tests, 1 suite-level error, 1 error, 45 failures, 1 ignored (1 assumption)
@ywelsch

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2016

I cannot reproduce this. @nik9000 what about you?

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

@ywelsch So if this works for do you think I can merge it?

@ywelsch

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2016

@dadoonet yes, go ahead.

@dadoonet dadoonet merged commit 5e1f26c into elastic:master Apr 29, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details
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.