Fail force-merges on read-only engines #64756

howardhuanghua · 2020-11-09T06:31:18Z

In our customer's production cluster, customer complains that some of the indices cannot do force merge to clean deleted docs.

health status index                           uuid                   pri rep docs.count docs.deleted store.size pri.store.size
green open  waf_log-2020.08.26              DblUYrcfQA2DsJzUH53QOg 12 0     13555 24101243   2.7gb   2.7gb
green open  waf_log-2020.08.28              mLv9TizPSuSFRwE0Y08vpQ 12 0     23456 30183641  12.7gb  12.7gb
green open  waf_log-2020.08.24              Iej4DBqcQPS7xdF1U6UZcA 12 0     55171 32343532   7.1gb   7.1gb
green open  waf_log-2020.08.23              qI51D1ObRTCNq84UnX9kEQ 12 0     27153 35677246   7.7gb   7.7gb

Here is the output of force merge api call:

POST /waf_log-2020.08.23/_forcemerge?max_num_segments=1&only_expunge_deletes=true&pretty
{
  "_shards" : {
    "total" : 12,
    "successful" : 12,
    "failed" : 0
  }
}

From _cat/segments api, we could see this index has more than one segments in each shard:

GET _cat/segments/waf_log-2020.08.23?v 
index              shard prirep ip            segment generation docs.count docs.deleted    size size.memory committed searchable version compound
waf_log-2020.08.23 0     p      172.16.138.28 _7lh          9845        421      1536889  16.1mb           0 true      false      8.3.0   false
waf_log-2020.08.23 0     p      172.16.138.28 _7lm          9850       1799      1767111    20mb           0 true      false      8.3.0   false
waf_log-2020.08.23 1     p      172.16.138.2  _7lu          9858       2261            0   5.7mb           0 true      false      8.3.0   false
waf_log-2020.08.23 2     p      172.16.138.28 _7h1          9685        547      1453000  15.7mb           0 true      false      8.3.0   false
waf_log-2020.08.23 2     p      172.16.138.28 _7h4          9688       1848      3775021 260.7mb           0 true      false      8.3.0   false
waf_log-2020.08.23 3     p      172.16.138.39 _7kd          9805        489      1601900  17.1mb           0 true      false      8.3.0   false
waf_log-2020.08.23 3     p      172.16.138.39 _7ki          9810       1843      1977995  33.7mb           0 true      false      8.3.0   false

We try to open low level log to check why force merge could not be executed:

PUT /_cluster/settings?pretty
{
  "transient": {
    "logger.org.elasticsearch.index.engine": "trace"
  }
}
'

But we got nothing output about the above index force merge operation. We cost long time to add extra log to figure out that the index has been frozen^^.

Currently even index has write block, we still allow user to execute force merge api, but I think we at least need to add some trace level log to let user know why the merge cannot be executed. So this PR add some useful low level log info to indicate index is read only during refresh, flush and force merge operations.

elasticmachine · 2020-11-09T10:11:06Z

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

DaveCTurner · 2020-11-09T10:18:14Z

A few thoughts:

requiring TRACE logging to investigate a user issue is a bug IMO, so I don't think this is the right fix.
Read-only engines do support flush and refresh (a no-op is the correct behaviour) so I think no change is required in those two cases. At least the proposed messages are misleading, but really I'd rather not have any message here at all.
I don't think we should consider a force-merge to be successful on a read-only engine (at least not unless it would have been a no-op anyway); throwing an exception would indicate its failure to the client which would be much preferable to relying on logs.

Adding the team-discuss label to gather others' thoughts on taking this forward.

howardhuanghua · 2020-11-09T15:57:34Z

Hi @DaveCTurner , I have updated the PR to support index write block for force merge API instead. Would you please help to review again?
Here is the output of force merge if index has write block:

// my-index-000001 is read only, my-index-000002 is writable
curl -XPOST localhost:9200/my-index-000001,my-index-000002/_forcemerge?pretty
{
  "error" : {
    "root_cause" : [
      {
        "type" : "cluster_block_exception",
        "reason" : "index [my-index-000001] blocked by: [FORBIDDEN/8/index write (api)];"
      }
    ],
    "type" : "cluster_block_exception",
    "reason" : "index [my-index-000001] blocked by: [FORBIDDEN/8/index write (api)];"
  },
  "status" : 403
}

DaveCTurner · 2020-11-09T16:04:00Z

That doesn't work either, it's fine to force-merge an index that merely has a write block. Indeed ILM applies such a block before force-merging.

Conversely there's no requirement for a frozen index to have a write block. It does by default but you can remove it.

howardhuanghua · 2020-11-10T02:26:59Z

Yes, I could remove the write block for frozen index. I have two questions:

A frozen index uses read only engine internally. Shall we need to set extra write block for it? Before removing write block, indexing doc would be rejected by write block exception. After removing write block, indexing doc would be rejected by read-only engine exception. Is that possible we could use read-only engine exception directly?
Force merge on frozen index (read-only engine) would do nothing (no-op) right? So we could just throw UnsupportedOperationException the same as index/delete operations?

DaveCTurner · 2020-11-11T14:51:58Z

We discussed this as a team and concluded that simply throwing an unconditional UnsupportedOperationException would be undesirable. For instance if you always force-merge to a single segment before freezing your indices then calling POST _all/_forcemerge?max_num_segments=1 manually should succeed. We saw two possible paths forward:

attempt an actual force-merge, but fail it if it tries to write anything on a read-only engine
find a rough approximation for whether a force-merge would be a no-op or not and only fail if not

We preferred the second idea: we think it would cover most cases to fail the merge iff the number of segments in the shard was greater than the max_num_segments parameter of the request. I also think there should be some DEBUG logs to describe what's going on.

howardhuanghua · 2020-11-11T15:48:27Z

find a rough approximation for whether a force-merge would be a no-op or not and only fail if not

So the rough approximation is that if shard already contains exactly max_num_segments specified number of segments, than we do no-op, if it has more segments than max_num_segments, just fail with UnsupportedOperationException?

DaveCTurner · 2020-11-11T16:05:48Z

contains exactly max_num_segments

Fewer is ok, otherwise yes that's right.

howardhuanghua · 2020-12-12T13:20:17Z

Hi @DaveCTurner , I have updated the PR, fail force merge if max_num_segments is fewer than current, otherwise do no-op. Would you please help to review?

DaveCTurner · 2020-12-16T11:21:10Z

@elasticmachine ok to test

DaveCTurner · 2020-12-16T11:45:18Z

Thanks for merging master @howardhuanghua :) I was just about to ask...

howardhuanghua · 2020-12-16T11:48:30Z

Thank you @DaveCTurner , since TencentCloudES repository is forked from elastic, this PR is merged from TencentCloudES to elastic, so I cannot tick the box to let maintainers push code to the branch. Next time I will branch from elastic repository directly, that would be worked.

DaveCTurner

Looks good, except for one missing corner case.

DaveCTurner · 2020-12-16T12:31:05Z

server/src/main/java/org/elasticsearch/index/engine/ReadOnlyEngine.java

@@ -375,6 +375,14 @@ public void flush(boolean force, boolean waitIfOngoing) throws EngineException {
    @Override
    public void forceMerge(boolean flush, int maxNumSegments, boolean onlyExpungeDeletes,
                           boolean upgrade, boolean upgradeOnlyAncientSegments, String forceMergeUUID) {
+        if (maxNumSegments < lastCommittedSegmentInfos.size()) {


This rejects force-merge requests that do not specify the maxNumSegments parameter at all, since that comes through to here as maxNumSegments == ForceMergeSegments.Defaults.MAX_NUM_SEGMENTS == -1. We should accept these requests too, it's ok for them to do nothing.

That's right, I have updated the change.

DaveCTurner

LGTM, thanks @howardhuanghua.

Today we treat all force-merges on a read-only (e.g. frozen) engine as no-ops, indicating to the client that they succeeded even if they had no effect. This commit corrects that behaviour, resolving the resulting confusion, by rejecting force-merges on read-only engines that are definitely not no-ops.

Today we treat all force-merges on a read-only (e.g. frozen) engine as no-ops, indicating to the client that they succeeded even if they had no effect. This commit corrects that behaviour, resolving the resulting confusion, by rejecting force-merges on read-only engines that are definitely not no-ops. Co-authored-by: Howard <[email protected]>

Add low level log info to indicate read only index blocking operations.

2ab926b

DaveCTurner added :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. team-discuss labels Nov 9, 2020

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Nov 9, 2020

howardhuanghua added 2 commits November 9, 2020 23:33

Support index write block for force merge.

f66faa1

add test case.

8ba6b7c

howardhuanghua changed the title ~~Add low level log info to indicate read only index blocking operations.~~ Support index write block for force merge API. Nov 9, 2020

unsupport force merge for frozen index

40ee1ef

howardhuanghua added 3 commits November 13, 2020 09:08

Merge branch 'master' into frozen_merge

7b6ca7f

Reject force merge if it's going to do real merge.

a0c3470

revert write block

8867481

DaveCTurner removed the team-discuss label Nov 18, 2020

howardhuanghua added 2 commits December 12, 2020 21:15

Merge master

c3a6eb1

Remove unsued version

f41b425

Merge remote-tracking branch 'upstream/master' into frozen_merge

c4a70a2

DaveCTurner added >bug v7.11.0 v8.0.0 labels Dec 16, 2020

DaveCTurner requested changes Dec 16, 2020

View reviewed changes

howardhuanghua added 2 commits December 16, 2020 23:06

Support default max num segments.

971790f

Merge remote-tracking branch 'upstream/master' into frozen_merge

f9327ba

pugnascotia added v7.12.0 and removed v7.11.0 labels Dec 16, 2020

DaveCTurner approved these changes Dec 17, 2020

View reviewed changes

DaveCTurner changed the title ~~Support index write block for force merge API.~~ Fail force-merges on read-only engines Dec 17, 2020

DaveCTurner merged commit 642b530 into elastic:master Dec 17, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail force-merges on read-only engines #64756

Fail force-merges on read-only engines #64756

howardhuanghua commented Nov 9, 2020 •

edited

Loading

elasticmachine commented Nov 9, 2020

DaveCTurner commented Nov 9, 2020 •

edited

Loading

howardhuanghua commented Nov 9, 2020 •

edited

Loading

DaveCTurner commented Nov 9, 2020 •

edited

Loading

howardhuanghua commented Nov 10, 2020

DaveCTurner commented Nov 11, 2020

howardhuanghua commented Nov 11, 2020

DaveCTurner commented Nov 11, 2020

howardhuanghua commented Dec 12, 2020

DaveCTurner commented Dec 16, 2020

DaveCTurner commented Dec 16, 2020

howardhuanghua commented Dec 16, 2020

DaveCTurner left a comment

DaveCTurner Dec 16, 2020

howardhuanghua Dec 16, 2020

DaveCTurner left a comment

Fail force-merges on read-only engines #64756

Fail force-merges on read-only engines #64756

Conversation

howardhuanghua commented Nov 9, 2020 • edited Loading

elasticmachine commented Nov 9, 2020

DaveCTurner commented Nov 9, 2020 • edited Loading

howardhuanghua commented Nov 9, 2020 • edited Loading

DaveCTurner commented Nov 9, 2020 • edited Loading

howardhuanghua commented Nov 10, 2020

DaveCTurner commented Nov 11, 2020

howardhuanghua commented Nov 11, 2020

DaveCTurner commented Nov 11, 2020

howardhuanghua commented Dec 12, 2020

DaveCTurner commented Dec 16, 2020

DaveCTurner commented Dec 16, 2020

howardhuanghua commented Dec 16, 2020

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner Dec 16, 2020

Choose a reason for hiding this comment

howardhuanghua Dec 16, 2020

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

howardhuanghua commented Nov 9, 2020 •

edited

Loading

DaveCTurner commented Nov 9, 2020 •

edited

Loading

howardhuanghua commented Nov 9, 2020 •

edited

Loading

DaveCTurner commented Nov 9, 2020 •

edited

Loading