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

Refreshes caused by realtime gets are not counted in the refresh stats #24806

Closed
nik9000 opened this issue May 19, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@nik9000
Copy link
Contributor

commented May 19, 2017

I was investigating something else and it looks like refreshes caused by realtime gets do not get factored into the refresh cycle. I was just looking around the code and that is how it looked. This reproduces the problem, I think:

DELETE /test

PUT /test
{
  "settings": {
    "refresh_interval": -1
  }
}

PUT /test/doc/1
{
  "test": "TEST"
}

GET /test/doc/1

GET /test/_stats/refresh

The refresh stats should show a refresh. Actually, it might be useful to track just how many of these we're getting in their own stat either on their own or in addition to refresh stats because many of them is indicative of a problem.

@bleskes

This comment has been minimized.

Copy link
Member

commented May 19, 2017

The refresh stats should show a refresh.

+1

Actually, it might be useful to track just how many of these we're getting in their own stat either on their own or in addition to refresh stats because many of them is indicative of a problem.

I wonder if we need this? What we really care about is how many refreshes happen per sec. Doesn't matter what caused them (and it's very easy to see what the refresh interval settings is).

@jasontedor

This comment has been minimized.

Copy link
Member

commented May 19, 2017

I agree, there is a bug here. The problem is the stats tracking is done for refresh calls that go through index shard, but not for refresh calls decided inside the engine.

Actually, it might be useful to track just how many of these we're getting in their own stat either on their own or in addition to refresh stats because many of them is indicative of a problem.

I do not think that we need this. A refresh is a refresh.

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2017

I do not think that we need this. A refresh is a refresh.

I'm not sure we need this either. If we had this then we could point to it and say "there, that is why you have so many refreshes" without having to dig deeply. It'd be an easy thing to tell people to check.

@nik9000 nik9000 closed this in 40a1334 Jun 6, 2017

nik9000 added a commit that referenced this issue Jun 6, 2017

Add refresh stats tracking for realtime get (#25052)
Passes a `LongConsumer` into the `Engine` during GETs which the engine
calls if it refreshed to perform the get.

Closes #24806

bleskes added a commit that referenced this issue Jun 7, 2017

Update `IndexShard#refreshMetric` via a `ReferenceManager.RefreshList…
…ener` (#25083)

The PR takes a different approach to solve #24806 than currently implemented via #25052. The `refreshMetric` that IndexShard maintains is updated using the refresh listeners infrastructure in lucene. This means that we truly count all refreshes that lucene makes and not have to worry about each individual caller (like `IndexShard@refresh` and `Engine#get()`)

bleskes added a commit that referenced this issue Jun 7, 2017

Update `IndexShard#refreshMetric` via a `ReferenceManager.RefreshList…
…ener` (#25083)

The PR takes a different approach to solve #24806 than currently implemented via #25052. The `refreshMetric` that IndexShard maintains is updated using the refresh listeners infrastructure in lucene. This means that we truly count all refreshes that lucene makes and not have to worry about each individual caller (like `IndexShard@refresh` and `Engine#get()`)
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.