-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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 option to _cat/indices
to return index creation date #11524
#11688
Conversation
Hi @szroland Thanks for the PR. I note that no tests have been added or updated (specifically the REST tests). Please could you add. thanks |
Hi @clintongormley, I added tests for the new fields to the default cat.indices test case. Cheers, |
Looks good to me. |
@nik9000 want to merge it in? |
@clintongormley sure. |
@szroland, I'm getting test failures:
I'll do a bit of debugging and then let you know what I find. Maybe locale issues? Those times look like they are printed in a pretty ISO8601-ish format so I wouldn't expect them to vary based on locale but, you know, computers are tricky beasts. |
Ok - DateTime's toString is documented as spitting out its results in ISO8601 which doesn't have that space between the date and the T. It doesn't look locale dependent. So, I revise my review: doesn't look good to me, remove the space in those regexes and then it will. |
I've dropped the review tag and added feedback_needed so we can track that this is waiting another (tiny) patch. I don't think feedback_needed is really the right label but its the closest one I can find. |
Returning index creation date, both as a numeric millisecond value and as a string. This implements elastic#11524
Interesting. Strictly speaking the error message makes sense since the date string does not contain that space. I did go ahead and remove it, also while at it brought the PR up in line with the latest commits. Still, this test wasn't failing for me, since the rest tests do pattern matching using the Pattern.COMMENTS flag, which supposedly allows comments and ignores white space in the regular expression. This seems to be the case for me on Windows with both a 1.7 and 1.8 Oracle jvm. E.g. the test passes with or without that space. Out of curiosity, what platform / JVM did you run this test on? |
Its OSX.
I'm not really sure what is up. I hadn't thought about the COMMENTS flag but its obvious from looking at the regexes it was on. I'll rerun the tests and recheck. |
/me shakes fist at time zones. Its because you are in a GMT + and I'm in a GMT -. It wasn't the space. One moment. |
@szroland I sent you a pull request for the branch you've used for this pull request. If you merge it it should fix the test for me and it'll become part of this pull request. |
[TESTS] cat indices times for UTC - timezones
Ah right, I didn't notice that either. Makes more sense than having weird consistency issues with the behavior of Pattern.COMMENTS. Merged your PR. |
Add option to `_cat/indices` to return index creation date #11524
OK! That looks great and everything passes now so I've merged it. Thanks @szroland! |
this change was added recently which uses default timezone for the creation date on CAT endpoints. We should be consistent and use UTC across the board. This commit adds #getDefaultTimzone() to forbidden API and fixes the REST tests. Relates to elastic#11688
Returning index creation date, both as a numeric millisecond value and
as a string. This implements #11524