Adds stats counter for failed indexing requests #13130

andrestc · 2015-08-26T23:09:43Z

#8938 - This adds a field index_failed to _stats showing how many index requests failed.

refactor adds tests refactor

nik9000 · 2015-08-27T13:18:17Z

core/src/main/java/org/elasticsearch/index/indexing/IndexingStats.java

@@ -155,6 +161,7 @@ public void readFrom(StreamInput in) throws IOException {
            indexCount = in.readVLong();
            indexTimeInMillis = in.readVLong();
            indexCurrent = in.readVLong();
+            indexFailedCount = in.readVLong();


I imagine we'll need the if (version.onOrAfter(Version.V_2_1_0)) dance around these reads and writes here. We didn't need it in 2.0 because we don't have multi-version compatibility but this will probably go to 2.1 and that'll have to be compatible with 2.0.

er - I think it goes like if (in.version().onOrAfter(Version.V_2_1_0)) or something like that.

This needs to be wrapping my change only, right?

Yup - just that read and write you inserted. Without it this isn't compatible with 2.0 servers. Usually people only run mixed version clusters during a rolling upgrade but its nice if everything continues to work then.

nik9000 · 2015-08-27T13:23:39Z

Looks pretty good. I left some comments about the test and compatibility with multiple node versions which I assume is a thing because this is (rightly) targeting master and not the 2.0 branch.

andrestc · 2015-08-27T14:19:46Z

@nik9000 I made some questions regarding some of your comments (mostly because im a new contributor so I don't know much about the codebase).
Will update my PR once I have the answers.

Thanks for the review.

nik9000 · 2015-08-27T14:21:02Z

Will update my PR once I have the answers.

I think I just answered actually. Good timing!

The best thing is to send a new commit that changes the changes and squash later (or not). Its easier to review that way.

andrestc · 2015-08-27T14:39:11Z

@nik9000 Just pushed it!
Thanks.

nik9000 · 2015-08-27T14:47:51Z

Looks good to me. I'd love to get a review from someone else just to be 100% sure. If someone doesn't show up in the next few hours I'll go hunting.

jasontedor · 2015-08-29T14:45:54Z

@andrestc Do you think that you can add this to the cat API as well?

andrestc · 2015-08-29T14:50:56Z

@jasontedor Sure, ill give it a shot and ask for help if i get stuck.

jasontedor · 2015-08-29T14:52:26Z

@andrestc Sounds good, thank you!

andrestc · 2015-08-30T03:38:15Z

@jasontedor There it is!

nik9000 · 2015-08-31T13:16:29Z

Thanks for remembering the _cat api, @jasontedor. I forgot it often.

nik9000 · 2015-08-31T14:03:39Z

Ok! I found another fun thing to test. You can test the backwards compatibility by writing an ESBackcompatTestCase. It looks like IndexingStats doesn't have one by NodeStats does. So you could use it as a starting point. That'd verify the need for that Version check.

@andrestc, are you up for it? You've been a great sport so far! You'll have to look into testing.asciidoc for instructions on running these. The python script that fetches the index is out of date on your branch so you'd need to run it on the master branch and switch back to your branch.

andrestc · 2015-08-31T17:43:13Z

@nik9000 I'll give it a shot during the week, sure!
Will get back to you if i get stuck.
Thanks!

andrestc · 2015-09-03T01:52:47Z

@nik9000 I'm having some problems to run the backwards tests.
First, On what version should i run against?
I followed the Testing.
I've tried it with 1.7.0 by doing the following:
After downloading, etc etc. I ran:

python ev-tools/create_bwc_index.py 1.7.0

When i run:

mvn test -Dtests.filter="@backwards" -Dtests.bwc.version=1.7.0 -Dtests.security.manager=false

Its fails and tells me there were no executed tests(i tried with "@Backwards", too). So i guess the problem is with the filter on mvn test.

Any ideas?

dakrone · 2015-09-09T14:53:54Z

This LGTM, I do agree with @nik9000 though that it would be nice to have BWC tests for IndexingStats

@nik9000 it looks like you opened an issue to fix the BWC test infrastructure, do you want to wait on this until that is fixed, or do the testing in a subsequent issue?

nik9000 · 2015-09-09T15:43:13Z

@nik9000 it looks like you opened an issue to fix the BWC test infrastructure, do you want to wait on this until that is fixed, or do the testing in a subsequent issue?

Either one is fine with me. @andrestc, if you are ok with me merging this as is I'll create a followup issue for the the backwards compatibility test. I don't want to block this pr forever just because we've broken part of our infrastructure.

andrestc · 2015-09-09T15:48:29Z

@nik9000 I'm definitely okay. As soon as the backwards infrastructure is okay I can submit a PR with this tests.

nik9000 · 2015-09-09T15:49:48Z

@nik9000 I'm definitely okay. As soon as the backwards infrastructure is okay I can submit a PR with this tests.

Cool. I'll merge it soon-ish then. I have 5 tabs worth of code reviews and merges to do when I stop concentrating on fixing the backwards compatibility tests.

nik9000 · 2015-09-10T13:49:29Z

OK! I picked this up to merge it this morning. I squashed it and reran the tests. One of the rest tests was failing so I'll ship a commit along with the merge that fixes it. It didn't merge 100% clean with master but the only conflict was obvious to resolve. I'll be rerunning the tests with the work staged on master locally and when all that passes I'll push to master. After that I'll backport to the 2.x branch which will eventually be used to form the 2.1 release.

nik9000 · 2015-09-10T14:14:25Z

Merged to master: 3839d15

nik9000 · 2015-09-10T14:37:21Z

And merged to 2.x: 1e347c3

This should show up in 2.1.0 whenever that is released.

nik9000 · 2015-09-10T14:43:38Z

Thanks so much for this @andrestc. Thanks for working through all the changes. I'm sorry you were the first person to hit the backwards compatibility tests being busted (#13425). We'll get those fixed up as soon as we can!

Thanks again!

andrestc · 2015-09-10T14:46:36Z

@nik9000 Np! Thanks for the review!

Adds counter for failed indexing requests

d2d9163

refactor adds tests refactor

andrestc changed the title ~~Adds counter for failed indexing requests~~ Adds stats counter for failed indexing requests Aug 26, 2015

clintongormley added >enhancement review :Data Management/Stats Statistics tracking and retrieval APIs labels Aug 27, 2015

nik9000 reviewed Aug 27, 2015
View reviewed changes

nik9000 added the v2.1.0 label Aug 27, 2015

nik9000 self-assigned this Aug 27, 2015

refactoring tests

6f27100

andrestc added 2 commits August 27, 2015 11:23

testing stats for both types on a same index

e2355cc

making sure to keep compatibility while writing/reading stats

c7b3f99

adds failed indexing stats to cat api

b68fdee

nik9000 mentioned this pull request Sep 9, 2015

Backwards compatibility tests broken for 2.x #13425

Closed

nik9000 added the v5.0.0-alpha1 label Sep 10, 2015

nik9000 closed this Sep 10, 2015

nik9000 mentioned this pull request Sep 10, 2015

Add backwards compatibility test for index failed stats #13474

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds stats counter for failed indexing requests #13130

Adds stats counter for failed indexing requests #13130

andrestc commented Aug 26, 2015

nik9000 Aug 27, 2015

nik9000 Aug 27, 2015

andrestc Aug 27, 2015

nik9000 Aug 27, 2015

nik9000 commented Aug 27, 2015

andrestc commented Aug 27, 2015

nik9000 commented Aug 27, 2015

andrestc commented Aug 27, 2015

nik9000 commented Aug 27, 2015

jasontedor commented Aug 29, 2015

andrestc commented Aug 29, 2015

jasontedor commented Aug 29, 2015

andrestc commented Aug 30, 2015

nik9000 commented Aug 31, 2015

nik9000 commented Aug 31, 2015

andrestc commented Aug 31, 2015

andrestc commented Sep 3, 2015

dakrone commented Sep 9, 2015

nik9000 commented Sep 9, 2015

andrestc commented Sep 9, 2015

nik9000 commented Sep 9, 2015

nik9000 commented Sep 10, 2015

nik9000 commented Sep 10, 2015

nik9000 commented Sep 10, 2015

nik9000 commented Sep 10, 2015

andrestc commented Sep 10, 2015

Adds stats counter for failed indexing requests #13130

Adds stats counter for failed indexing requests #13130

Conversation

andrestc commented Aug 26, 2015

nik9000 Aug 27, 2015

Choose a reason for hiding this comment

nik9000 Aug 27, 2015

Choose a reason for hiding this comment

andrestc Aug 27, 2015

Choose a reason for hiding this comment

nik9000 Aug 27, 2015

Choose a reason for hiding this comment

nik9000 commented Aug 27, 2015

andrestc commented Aug 27, 2015

nik9000 commented Aug 27, 2015

andrestc commented Aug 27, 2015

nik9000 commented Aug 27, 2015

jasontedor commented Aug 29, 2015

andrestc commented Aug 29, 2015

jasontedor commented Aug 29, 2015

andrestc commented Aug 30, 2015

nik9000 commented Aug 31, 2015

nik9000 commented Aug 31, 2015

andrestc commented Aug 31, 2015

andrestc commented Sep 3, 2015

dakrone commented Sep 9, 2015

nik9000 commented Sep 9, 2015

andrestc commented Sep 9, 2015

nik9000 commented Sep 9, 2015

nik9000 commented Sep 10, 2015

nik9000 commented Sep 10, 2015

nik9000 commented Sep 10, 2015

nik9000 commented Sep 10, 2015

andrestc commented Sep 10, 2015