Cache modification time of translog writer file #95107

loupipalien · 2023-04-10T12:31:07Z

Usually, several indices are written even the cluster has a lot of indices, so we can cache modified time of translog writer file which already not be written to reduce cost time of nodes and indices stats

… cost time of nodes and indices stats Change-Id: I74ddbce3cdd8f4c3263a8cd51fa8b76bd097ae17

elasticsearchmachine · 2023-04-11T09:30:20Z

Pinging @elastic/es-distributed (Team:Distributed)

Change-Id: I241b669b6913a94a7d41ca08dade693663494ca6

loupipalien · 2023-04-17T04:25:25Z

@astefan can you help to invite a reviewer for this pr

loupipalien · 2023-04-19T08:33:26Z

we has a large cluster which more than 4k shards per node, as this cluster use network file system, getting mtime of a file from fs cost avg about 0.7ms ~ 0.8ms

after this optimize, only a few of translog writer file that is writting need to get mtime from fs，other translog writer files get mtime from memory cache cost avg just 0.01 ~ 0.02ms

kingherc

Thanks for your contribution! Left a few comments to consider. But overall makes sense to me to cache this value.

kingherc · 2023-04-20T09:28:28Z

server/src/main/java/org/elasticsearch/index/translog/TranslogWriter.java

+
+ @Override
+ protected boolean needsRefresh() {
+ LastModifiedTime cached = getNoRefresh();


Since the modification time is connected to the written file, I wonder if it would be better to judge the need for refresh based on the getWrittenOffset() value (which is directly got from the file channel that the translog writes to) rather than on the combination of operationCounter and lastSyncedCheckpoint?

@kingherc thanks for your time and patient. I think it wouldn't be better if we judge the need for refresh base on getWrittenOffset, because getWrittenOffset also invokes a syscall every time that is same as BaseTranslogReader#getLastModifiedTime

server/src/main/java/org/elasticsearch/index/translog/TranslogWriter.java

kingherc · 2023-04-20T09:32:32Z

server/src/main/java/org/elasticsearch/index/translog/TranslogWriter.java

+ return lastModifiedTime.getOrRefresh().lastModifiedTime;
+ } catch (UncheckedIOException e) {
+ // wrapped in the cache and unwrap here
+ throw e.getCause();


Rather than using a SingleObjectCache, for which you basically do not use the time refresh functionality, I wonder if simply extending the LastModifiedTime record with a similar function .getOrRefresh() would be more straightforward and would also enable throwing the involved IOException directly rather than doing this juggling with the UncheckedIOException.

org.elasticsearch.index.translog.TranslogReader#getLastModifiedTime also has such a simple caching (rather than using the SingleObjectCache).

Got it, fixed

…SingleObjectCache Change-Id: I723a19f282ff261ffd3bf1124f4fd91d571e6320

kingherc

Some final comments.

Could you also add some testing around the new last modified time behavior? E.g., in server/src/test/java/org/elasticsearch/index/translog/TranslogTests.java there are some tests around TranslogWriter and you could assert that the last modified time is modified once there's a sync of the translog or some operations added.

Thanks again!

kingherc · 2023-04-24T11:35:01Z

server/src/main/java/org/elasticsearch/index/translog/TranslogWriter.java

+
+ @Override
+ public long getLastModifiedTime() throws IOException {
+ if (lastModifiedTime.totalOffset() != totalOffset || lastModifiedTime.syncedOffset() != lastSyncedCheckpoint.offset) {


Delved a bit more into the code. I think this is correct apart from one weird edge case I see: org.elasticsearch.index.translog.TranslogWriter#readBytes() seems to be writing buffered ops into the channel file without touching totalOffset nor lastSyncedCheckpoint. Would it make sense to reset the lastModifiedTime (e.g., set its totalOffset to -1 in readBytes() in that if statement?

I find usage of org.elasticsearch.index.translog.TranslogWriter#getLastModifiedTime()，the last mtime of writer file is only used to find earliest last modified age of translog files to construct translog stats，so it seems not need an exact value
If only judge the need for refresh based on lastSyncedCheckpoint, the cached last mtime maybe lag too much when translog durability is set async and sync_interval is set too large, so adding totalOffset condition to get mtime more frequent to decrease the lag
I think writing buffered ops into channel may not change mtime, so not need set totalOffset to -1 in readBytes() to keep cache logic simple

Indeed I see it's only used in stats, so I agree there is no need to make it more complicated for this rather unusual edge case.

server/src/main/java/org/elasticsearch/index/translog/TranslogWriter.java

kingherc · 2023-04-24T11:38:15Z

@elasticmachine test this please

Change-Id: I26330eee653ba13588669c24b55e354e33d28f60

loupipalien · 2023-04-26T19:24:15Z

Could you also add some testing around the new last modified time behavior? E.g., in server/src/test/java/org/elasticsearch/index/translog/TranslogTests.java there are some tests around TranslogWriter and you could assert that the last modified time is modified once there's a sync of the translog or some operations added.

@kingherc I add a test, thanks again for you time and patient ❤️

kingherc · 2023-04-27T12:06:02Z

@elasticmachine test this please

kingherc

Thank you for the PR! Feel free to merge it.

Change-Id: I99c0b28a2da7ab69d0e100485fed406ef698156d

…/elasticsearch into cache_translog_stats Change-Id: I52d20ad9242b4265404442670554545a826bb057

kingherc · 2023-04-28T07:18:06Z

@elasticmachine test this please

kingherc · 2023-04-28T09:42:45Z

@loupipalien I merged this. Feel free to tell me if there was anything else remaining and we can do another PR. Thanks for this!

Cache mtime of translog writer which hasn't write operation to reduce…

0120ac7

… cost time of nodes and indices stats Change-Id: I74ddbce3cdd8f4c3263a8cd51fa8b76bd097ae17

elasticsearchmachine added needs:triage Requires assignment of a team area label v8.8.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Apr 10, 2023

loupipalien changed the title ~~Cache mtime of translog writer which hasn't write operation to reduce…~~ Cache mtime of translog writer file which not be written anymore Apr 10, 2023

astefan added :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. and removed needs:triage Requires assignment of a team area label labels Apr 11, 2023

elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Apr 11, 2023

Add docs/changelog/95107.yaml

f988d76

Change-Id: I241b669b6913a94a7d41ca08dade693663494ca6

kingherc reviewed Apr 20, 2023

View reviewed changes

kingherc self-assigned this Apr 20, 2023

kingherc added the >non-issue label Apr 20, 2023

kingherc requested a review from Tim-Brooks April 20, 2023 09:37

Using a record and simply overwriting getLastModifiedTime instead of …

22780ac

…SingleObjectCache Change-Id: I723a19f282ff261ffd3bf1124f4fd91d571e6320

loupipalien requested a review from kingherc April 23, 2023 08:51

kingherc reviewed Apr 24, 2023

View reviewed changes

gmarouli added v8.9.0 and removed v8.8.0 labels Apr 26, 2023

loupipalien closed this Apr 26, 2023

loupipalien reopened this Apr 26, 2023

Add a test

7f6874b

Change-Id: I26330eee653ba13588669c24b55e354e33d28f60

loupipalien requested a review from kingherc April 27, 2023 10:15

Update 95107.yaml

8d81a48

kingherc approved these changes Apr 27, 2023

View reviewed changes

loupipalien changed the title ~~Cache mtime of translog writer file which not be written anymore~~ Cache modification time of translog writer file Apr 27, 2023

loupipalien added 2 commits April 28, 2023 13:52

Fix add ops greater than zero

f0d3c98

Change-Id: I99c0b28a2da7ab69d0e100485fed406ef698156d

Merge branch 'cache_translog_stats' of https://github.com/loupipalien…

d6d402b

…/elasticsearch into cache_translog_stats Change-Id: I52d20ad9242b4265404442670554545a826bb057

kingherc merged commit 12447ce into elastic:main Apr 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cache modification time of translog writer file #95107

Cache modification time of translog writer file #95107

Uh oh!

loupipalien commented Apr 10, 2023

elasticsearchmachine commented Apr 11, 2023

loupipalien commented Apr 17, 2023

loupipalien commented Apr 19, 2023 •

edited

Loading

kingherc left a comment

kingherc Apr 20, 2023

loupipalien Apr 23, 2023

Uh oh!

kingherc Apr 20, 2023

loupipalien Apr 23, 2023

kingherc left a comment

kingherc Apr 24, 2023

loupipalien Apr 26, 2023 •

edited

Loading

kingherc Apr 27, 2023

Uh oh!

kingherc commented Apr 24, 2023

loupipalien commented Apr 26, 2023 •

edited

Loading

kingherc commented Apr 27, 2023

kingherc left a comment

kingherc commented Apr 28, 2023

kingherc commented Apr 28, 2023

Labels

5 participants

Cache modification time of translog writer file #95107

Cache modification time of translog writer file #95107

Uh oh!

Conversation

loupipalien commented Apr 10, 2023

elasticsearchmachine commented Apr 11, 2023

loupipalien commented Apr 17, 2023

loupipalien commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

kingherc left a comment

Choose a reason for hiding this comment

kingherc Apr 20, 2023

Choose a reason for hiding this comment

loupipalien Apr 23, 2023

Choose a reason for hiding this comment

Uh oh!

kingherc Apr 20, 2023

Choose a reason for hiding this comment

loupipalien Apr 23, 2023

Choose a reason for hiding this comment

kingherc left a comment

Choose a reason for hiding this comment

kingherc Apr 24, 2023

Choose a reason for hiding this comment

loupipalien Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

kingherc Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

kingherc commented Apr 24, 2023

loupipalien commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

kingherc commented Apr 27, 2023

kingherc left a comment

Choose a reason for hiding this comment

kingherc commented Apr 28, 2023

kingherc commented Apr 28, 2023

Labels

5 participants

loupipalien commented Apr 19, 2023 •

edited

Loading

loupipalien Apr 26, 2023 •

edited

Loading

loupipalien commented Apr 26, 2023 •

edited

Loading