Skip to content

Move IndexShard#getWritingBytes() under InternalEngine #27209

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

Merged
merged 3 commits into from
Nov 2, 2017

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Nov 1, 2017

We do some accouting in IndexShard that is not necessarily correct since
we maintain two different index readers. This change moves the accounting under
the engine which knows what reader we are refreshing.

Relates to #26972

We do some accouting in IndexShard that is not necessarily correct since
we maintain two different index readers. This change moves the accounting under
the engine which knows what reader we are refreshing.

Relates to elastic#26972
Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

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

I left one important comment in index shard. It's a shame we don't have unit tests for this, but this is an existing problem, so I don't want to hold the PR for this.

@@ -842,19 +836,7 @@ public void refresh(String source) {
verifyNotClosed();

if (canIndex()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

empty if clause? also, canIndex always returns true ... so some test should have failed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I am not sure something should have in this case there are tests for the entire thing but they are not very sophisticated. I also think it's very hard to test.

@@ -410,6 +416,14 @@ public String getHistoryUUID() {
}

/**
* Returns how many bytes we are currently moving from heap to disk
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: shall we be more concrete and say how much bytes are moving from the indexing buffer to segments?

@s1monw
Copy link
Contributor Author

s1monw commented Nov 1, 2017

@bleskes I pushed changes

Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

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

thx @s1monw

@s1monw s1monw merged commit f928d61 into elastic:master Nov 2, 2017
s1monw added a commit that referenced this pull request Nov 2, 2017
We do some accounting in IndexShard that is not necessarily correct since
we maintain two different index readers. This change moves the accounting under
the engine which knows what reader we are refreshing.

Relates to #26972
jasontedor added a commit to original-brownbear/elasticsearch that referenced this pull request Nov 2, 2017
* master:
  Remove unused searcher parameter in SearchService#createContext (elastic#27227)
  Upgrade to Lucene 7.1 (elastic#27225)
  Move IndexShard#getWritingBytes() under InternalEngine (elastic#27209)
  Adjust bwc version for exists query tests
martijnvg added a commit that referenced this pull request Nov 3, 2017
* master:
  Fixed byte buffer leak in Netty4 request handler
  Avoid uid creation in ParsedDocument (#27241)
  Rander sum as zero if count is zero for stats aggregation (#26893) (#27193)
  Add additional explanations around discovery.zen.ping_timeout (#27231)
  Remove unused searcher parameter in SearchService#createContext (#27227)
  Upgrade to Lucene 7.1 (#27225)
  Move IndexShard#getWritingBytes() under InternalEngine (#27209)
  Adjust bwc version for exists query tests
  Introducing took time for _msearch
martijnvg added a commit that referenced this pull request Nov 3, 2017
* 6.x:
  Fixed byte buffer leak in Netty4 request handler
  Avoid uid creation in ParsedDocument (#27241)
  Upgrade to Lucene 7.1 (#27225)
  Add additional explanations around discovery.zen.ping_timeout (#27231)
  Fix compile error
  Remove unused searcher parameter in SearchService#createContext (#27227)
  Fix sequence number assertions in BWC tests
  Move IndexShard#getWritingBytes() under InternalEngine (#27209)
  Adjust bwc version for exists query tests
@clintongormley clintongormley added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. and removed :Engine :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. labels Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >enhancement v6.1.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants