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

Add primitive to shrink an index into a single shard #18270

Merged
merged 23 commits into from May 31, 2016

Conversation

Projects
None yet
6 participants
@s1monw
Copy link
Contributor

commented May 11, 2016

This adds a low level primitive operations to shrink an existing
index into a new index with a single shard. This primitive expects
all shards of the source index to allocated on a single node. Once the target index is initializing on the shrink node it takes a snapshot of the source index shards and copies all files into the target indices data folder. An optimization coming in Lucene 6.1 will also allow for optional constant time copy if hard-links are supported by the filesystem. All mappings are merged into the new indexes metadata once the snapshots have been taken on the merge node.

To shrink an existing index all shards must be moved to a single node (one instance of each shard) and the index must be read-only:

$ curl -XPUT 'http://localhost:9200/logs/_settings' -d '{
    "settings" : {
        "index.routing.allocation.require._name" : "shrink_node_name",
        "index.blocks.write" : true 
    }
}

once all shards are started on the shrink node. the new index can be created via:

$ curl -XPUT 'http://localhost:9200/logs/_shrink/logs_single_shard' -d '{
    "settings" : {
        "index.codec" : "best_compression",
        "index.number_of_replicas" : 1
    }
}'

This API will perform all needed check before the new index is created and selects the shrink node based on the allocation of the source index. This call returns immediately, to monitor shrink progress the recovery API should be used since all copy operations are reflected in the recovery API with byte copy progress etc.

The shrink operation does not modify the source index, if a shrink operation should
be canceled or if the shrink failed, the target index can simply be deleted and
all resources are released.

@mikemccand

View changes

core/src/main/java/org/elasticsearch/index/shard/StoreRecovery.java Outdated
@@ -200,15 +253,13 @@ private void internalRecoverFromStore(IndexShard indexShard, boolean indexShould
// it exists on the directory, but shouldn't exist on the FS, its a leftover (possibly dangling)
// its a "new index create" API, we have to do something, so better to clean it than use same data
logger.trace("cleaning existing shard, shouldn't exists");
IndexWriter writer = new IndexWriter(store.directory(), new IndexWriterConfig(Lucene.STANDARD_ANALYZER).setOpenMode(IndexWriterConfig.OpenMode.CREATE));
writer.close();

This comment has been minimized.

Copy link
@mikemccand

mikemccand May 11, 2016

Contributor

I'm glad you removed this scariness ... does this (dangling index) ever happen? Aren't we more careful about picking unique directories? Can we throw AssertionError if we get here? Or at least remove the logging line :)

This comment has been minimized.

Copy link
@s1monw

s1monw May 12, 2016

Author Contributor

I think that was an accident but I will look into removing this later

@mikemccand

View changes

core/src/main/java/org/elasticsearch/index/store/Store.java Outdated
} else {
super.copyFrom(from, srcFile, destFile, context);
}
}
}

This comment has been minimized.

Copy link
@mikemccand

mikemccand May 11, 2016

Contributor

I wonder if Lucene's Directory.copyFrom should use hard links (optimization) when it can? Seems crazy you have to optimize this yourself.

This comment has been minimized.

Copy link
@s1monw

s1monw May 12, 2016

Author Contributor

+1 I wonder what @uschindler @rmuir think about that?

This comment has been minimized.

Copy link
@uschindler

uschindler May 12, 2016

Contributor

I think we had the discussion already on the Lucene mailing lists. I have no strong feelings, but hard links are often something bad to system administrators. So there should be at least some way to opt out.

I could think about having that on the Lucene side, but it would need the same strange workaround. unwrapDirectory is a no-go inside Lucene, sorry, because it may break very easy for customized or filtered directories.

@mikemccand

View changes

core/src/main/java/org/elasticsearch/index/shard/LocalShardSnapshot.java Outdated
public Directory getSnapshotDirectory() {
/* this directory will not be used for anything else but reading / copying files to another directory
* we prevent all write operations on this directory with UOE - nobody should close it either. */
return new FilterDirectory(store.directory()) {

This comment has been minimized.

Copy link
@mikemccand

mikemccand May 11, 2016

Contributor

Neat :)

@mikemccand

View changes

core/src/main/java/org/elasticsearch/index/store/Store.java Outdated
public void copyFrom(Directory from, String srcFile, String destFile, IOContext context) throws IOException {
Directory fromUnwrapped = FilterDirectory.unwrap(from);
Directory toUnwrapped = FilterDirectory.unwrap(this);
// try to unwrap to FSDirectory - we might be able to just create hard-links of these files and safe copying

This comment has been minimized.

Copy link
@mikemccand

mikemccand May 11, 2016

Contributor

safe -> save

@mikemccand

View changes

core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java Outdated
@@ -383,6 +383,14 @@ public MappingMetaData mapping(String mappingType) {
return mappings.get(mappingType);
}

public static final Setting<String> INDEX_MERGE_SOURCE_UUID = Setting.simpleString("index.merge.source.uuid");

This comment has been minimized.

Copy link
@mikemccand

mikemccand May 11, 2016

Contributor

I wonder if we can find a different name than merge since it's somewhat overloaded? collapse maybe?

This comment has been minimized.

Copy link
@s1monw

s1monw May 11, 2016

Author Contributor

yeah shrink maybe

This comment has been minimized.

Copy link
@mikemccand

mikemccand May 11, 2016

Contributor

+1 for shrink!

This comment has been minimized.

Copy link
@bleskes

bleskes May 12, 2016

Member

don't we need an IndexScope here?

This comment has been minimized.

Copy link
@s1monw

s1monw May 12, 2016

Author Contributor

no the uuid is only internal

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented May 11, 2016

LGTM ... I'm surprised how small a change this was.

@clintongormley clintongormley changed the title Add primitive to merge and index into a single shard Add primitive to merge an index into a single shard May 12, 2016

@bleskes

View changes

core/src/main/java/org/elasticsearch/index/shard/StoreRecovery.java Outdated
SegmentInfos si = store.readLastCommittedSegmentsInfo();
final RecoveryState.Index index = indexShard.recoveryState().getIndex();
final Directory directory = store.directory();
for (String name : Lucene.files(si)) {

This comment has been minimized.

Copy link
@bleskes

bleskes May 12, 2016

Member

This code can be shared by the two recoveries (from shards and local), types no?

@bleskes

View changes

core/src/main/java/org/elasticsearch/index/shard/StoreRecovery.java Outdated
indexShard.mapperService().merge(mapping.key,mapping.value.source(), MapperService.MergeReason.MAPPING_RECOVERY, true);
}
return executeRecovery(indexShard, () -> {
logger.debug("starting recovery from local shards {}", shards);

This comment has been minimized.

Copy link
@bleskes

bleskes May 12, 2016

Member

I think we need a toString LocalShardSnapshot, no?

This comment has been minimized.

Copy link
@s1monw

s1monw May 12, 2016

Author Contributor

yeah true

@bleskes

View changes

core/src/main/java/org/elasticsearch/index/store/Store.java Outdated
throw ex; // in these cases we bubble up since it's a true error condition.
} catch (IOException
| UnsupportedOperationException // if the FS doesn't support hard-links
| SecurityException ex // we might miss a java.nio.file.LinkPermission" "hard"

This comment has been minimized.

Copy link
@bleskes

bleskes May 12, 2016

Member

should we be defensive on this? I mean we explicitly allow for this permission - we might be hiding something else with this?

This comment has been minimized.

Copy link
@s1monw

s1monw May 12, 2016

Author Contributor

there is a test for this!

@bleskes

View changes

core/src/main/java/org/elasticsearch/index/store/Store.java Outdated
| UnsupportedOperationException // if the FS doesn't support hard-links
| SecurityException ex // we might miss a java.nio.file.LinkPermission" "hard"
) {
// hard-links are not supported or the files are on different filesystems

This comment has been minimized.

Copy link
@bleskes

bleskes May 12, 2016

Member

do we want some kind of logging here?

This comment has been minimized.

Copy link
@s1monw

s1monw May 12, 2016

Author Contributor

no that is too low level for logging - it's the old discussion I guess

@bleskes

View changes

core/src/main/java/org/elasticsearch/indices/cluster/IndicesClusterStateService.java Outdated
shardStateAction.shardStarted(shardRouting, "after recovery from store", SHARD_STATE_ACTION_LISTENER);
Index mergeSourceIndex = indexShard.indexSettings().getIndexMetaData().getMergeSourceIndex();
if (mergeSourceIndex != null) {
IndexService sourceIndexService = indicesService.indexService(mergeSourceIndex);

This comment has been minimized.

Copy link
@bleskes

bleskes May 12, 2016

Member

do we need to protect against the index not being there? right now it will blow up on the cluster state update thread with an NPE? we assume this primitive will be called when the source shard is assigned to the same node, but we either need to enforce somehow or protect properly against this not being the case.

This comment has been minimized.

Copy link
@s1monw

s1monw May 12, 2016

Author Contributor

yeah I already fixed that in my local copies - I am adding tests though

@bleskes

View changes

core/src/main/java/org/elasticsearch/indices/cluster/IndicesClusterStateService.java Outdated
for (IndexShard shard : sourceIndexService) {
if (shard.state() == IndexShardState.STARTED) {
startedShards.add(shard);
} else if (shard.state() == IndexShardState.POST_RECOVERY) {

This comment has been minimized.

Copy link
@bleskes

bleskes May 12, 2016

Member

do we really need to allow for this and have the added complexity? did you have to do this because of the order in which apply the same cluster state , causing the source shards to be moved to started after the initializing shard? we can maybe change the order of applying the cluster state, or otherwise just assume it's ok here (maybe add an assertion) and let thing explode later on when the shards tell us their engines are not started?

This comment has been minimized.

Copy link
@s1monw

s1monw May 12, 2016

Author Contributor

I am already working on a change that does that, do initializing ones last

@bleskes

View changes

core/src/main/java/org/elasticsearch/indices/cluster/IndicesClusterStateService.java Outdated
});
});
} else {
logger.debug("not all shards from index {} are started yet, expected {} found {} can't recover merge shard {}",

This comment has been minimized.

Copy link
@bleskes

bleskes May 12, 2016

Member

same question about whether we need to account for this.

@bleskes

View changes

core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java Outdated
@@ -1126,4 +1126,12 @@ public void onSettingsChanged() {
public MergeStats getMergeStats() {
return mergeScheduler.stats();
}

@Override
public final void addIndices(Directory... directories) throws IOException {

This comment has been minimized.

Copy link
@bleskes

bleskes May 12, 2016

Member

nit: since we call indexWriter.addIndexes, maybe use the same name here? alternatively we can de addDirectories, which might be a better name from an ES context (as Index means something else)

@bleskes

This comment has been minimized.

Copy link
Member

commented May 12, 2016

I left some minor comments. I'm also happy it's so small. IndexWriter.addIndexes is neat. May biggest feedback is whether we need to make the shard initialization be lenient in waiting for it's source shards to appear. Maybe we should require a different operation order for using this primitive:

  • move all shards to one node
  • create and index and force the shard to be in the same node

This will allows us to remove the waiting make it a hard failure if the shards are missing.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2016

Maybe we should require a different operation order for using this primitive:

I tell you it's all trappy, you create the index and somebody relocates a shard what do you do then. The semantics are pretty clear here as they are for that exact reason. I tried so many models with picking the right node, moving stuff first, then allocate, copy mapping on merge index creation etc. there are so many moving parts which makes stuff like this crazy trappy. There is literally no waiting here, we only even start recovering once all shards are are there and started, there is no waiting or anything, everything is implicit. I am afraid of doing something like your suggestion since it will cause users to make assumptions. We can't guarantee those in a dist system where basically everything is concurrent and nothing fails if you do it afterwards.

@rmuir

View changes

core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java Outdated
final Policy template;
final Policy untrusted;
final Policy system;
final PermissionCollection dynamic;
final Map<String,Policy> plugins;

public ESPolicy(PermissionCollection dynamic, Map<String,Policy> plugins, boolean filterBadDefaults) {
dynamic.add(new LinkPermission("hard")); // nocommit - is this the right place to add this?

This comment has been minimized.

Copy link
@rmuir

rmuir May 12, 2016

Contributor

yeah the link permission is a particularly bad one. it means you can access all files because of how permissions work on unix. I think its a good idea to contain it to just a particular piece of code rather than globally granted. In all cases it should be handled in the policy configuration file.

I think it can still work out though: personally, I don't think we should implement Store.java's copyFile with hard links on the regular basis anyway, instead I think its better implemented as a FilterDirectory that implements it this way solely for addIndexes (it seems to be a good tradeoff here to reduce transient space for that operation, but otherwise I think we should just copy bytes the simple way).

This comment has been minimized.

Copy link
@rmuir

rmuir May 12, 2016

Contributor

btw a practical thing might be, add this "merge wrapper" directory with the hard link logic to lucene-misc module. There are other crazy Directories in there already :) Also there is an IndexMergeTool there (https://github.com/apache/lucene-solr/blob/master/lucene/misc/src/java/org/apache/lucene/misc/IndexMergeTool.java) and we could wrap with this hard-link directory by default, so the tool uses less transient space. Maybe removing its forceMerge(1) would also help :)

Just seems to be an ok home for it, to keep things contained.

This comment has been minimized.

Copy link
@s1monw

@s1monw s1monw force-pushed the s1monw:feature/shard_merging branch May 13, 2016

@s1monw s1monw force-pushed the s1monw:feature/shard_merging branch May 20, 2016

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2016

@bleskes i pushed new changes with your idea of failing the shard - maybe you can take a quick look

@bleskes

View changes

core/src/main/java/org/elasticsearch/index/shard/IndexShard.java Outdated
IndexMetaData metaData = sourceIndexService.getMetaData();
int numShards = metaData != null ? metaData.getNumberOfShards() : -1;
final List<IndexShard> startedShards = new ArrayList<>();
IndexMetaData sourceIndexMetaData = indicesService.getIndexMetaData(mergeSourceIndex);

This comment has been minimized.

Copy link
@bleskes

bleskes May 23, 2016

Member

can we get the metaData from IndexService.getIndexSettings? This will mean IndicesService doesn't need to proxy clusterService.state()

new RecoveryListener(shardRouting, indexService), repositoriesService);
new RecoveryListener(shardRouting, indexService), repositoriesService, (type, mapping) -> {
try {
nodeServicesProvider.getClient().admin().indices().preparePutMapping()

This comment has been minimized.

Copy link
@bleskes

bleskes May 23, 2016

Member

this is tricky - how do we secure this for example? . I wonder if we should go through a simpler route where the master copies the mapping from the source index to the target index when resolving INDEX_SHRINK_SOURCE_NAME . The IndexShard can just verify that the source and target mapping are identical, and if not fail the allocation?

This comment has been minimized.

Copy link
@s1monw

s1monw May 23, 2016

Author Contributor

The IndexShard can just verify that the source and target mapping are identical, and if not fail the allocation?

it can't it has to be the master.

this is tricky - how do we secure this for example?

this is the ideal situation, even if you keep on writing to the index at the time we copy the mapping we already got a snapshot of the shard and the docs that are in the snapshot are guaranteed in the mapping. You are prone to races if you copy ahead of time and you are prone to races if you check. I am sorry your suggestion make it error prone and more complex. ES is inherent concurrent which makes anything hard. Unless we fix that we can't make such assumptions.

This comment has been minimized.

Copy link
@bleskes

bleskes May 23, 2016

Member

@s1monw talked about this offline - I'm good to go and see how things develop.

@bleskes

This comment has been minimized.

Copy link
Member

commented May 23, 2016

@s1monw I like how the retry_failed simplified things. Thank you for doing that. I left another suggestion w.r.t mapping. Let me know what you think.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2016

I like how the retry_failed simplified things.

it complicated things, sorry. you really need to know when all the relocaiton is done rather than make it pick things up automatically. this is much more complex IMO. you have to deal with retries etc. which sucks.

@bleskes

This comment has been minimized.

Copy link
Member

commented May 23, 2016

+1

throw new IllegalArgumentException("mappings are not allowed when shrinking indices" +
", all mappings are copied from the source index");
}
final Settings tragetIndexSettings = Settings.builder().put(targetIndex.settings())

This comment has been minimized.

Copy link
@bleskes

bleskes May 27, 2016

Member

s/tragetIndexSettings/targetIndexSettings

}
final Settings tragetIndexSettings = Settings.builder().put(targetIndex.settings())
.normalizePrefix(IndexMetaData.INDEX_SETTING_PREFIX).build();
if (IndexMetaData.INDEX_NUMBER_OF_SHARDS_SETTING.exists(tragetIndexSettings)

This comment has been minimized.

Copy link
@bleskes

bleskes May 27, 2016

Member

this validation is duplicated in the MetaDataCreateIndexService including some more checks regarding the source index. I think we should just have one place - either here or there. I think both works.

.put("index.routing.allocation.include._id", Strings.arrayToCommaDelimitedString(nodesToAllocateOn.toArray()))
// we only try once and then give up with a shrink index
.put("index.allocation.max_retries", 1)
// now copy all similarity / analysis settings - this overrides all settings from the user unless they wanna add extra settings

This comment has been minimized.

Copy link
@bleskes

bleskes May 27, 2016

Member

I wonder if we should be stricter and require no analysisSimilarity settings are on the target node - o.w. who knows what kind of mixture we'll get? Thinking more about it - should we just white list the settings that are OK to change? like compression and grow the list as we go?

This comment has been minimized.

Copy link
@s1monw

s1monw May 27, 2016

Author Contributor

all analysis / similarity settings that are relevant are copied. If we get additional ones we only get what we would be if somebody closes the index which is valid. I think we are completely fine here.

}

@Override
public ActionRequestValidationException validate() {

This comment has been minimized.

Copy link
@bleskes

bleskes May 27, 2016

Member

since we expose the underlying createIndexRequest, we should validate it too. Also we can do some early validation here like the number of shards and having no mapping etc. Last, I wonder if things will be simpler conceptually if ShrinkRequest will extend CreateIndexRequest and have all it's properties directly accessible (and adding one "SourceIndex" property)

This comment has been minimized.

Copy link
@s1monw

s1monw May 27, 2016

Author Contributor

yeah so I had this idea too... there are tricky generics related to AcknowledgedRequest that prevents this from being simple :( if we only had NO builders!!!

This comment has been minimized.

Copy link
@bleskes

bleskes May 30, 2016

Member

😞 too bad. I do think it's good to validate the underlying requests as well here?

This comment has been minimized.

Copy link
@s1monw

s1monw May 30, 2016

Author Contributor

I do that?

This comment has been minimized.

Copy link
@bleskes

bleskes May 30, 2016

Member

oh, missed the change to the first line as well. all good then, sorry for the noise.

logger.debug("starting recovery from local shards {}", shards);
try {
/*
* TODO: once we upgraded to Lucene 6.1 use HardlinkCopyDirectoryWrapper to enable hardlinks if possible and enable it

This comment has been minimized.

Copy link
@bleskes

bleskes May 27, 2016

Member

can we add a static code assertion to make sure we don't forget?

This comment has been minimized.

Copy link
@s1monw

s1monw May 27, 2016

Author Contributor

we don't do this anymore since all kinds tests will fail then... I can write a test that fails instead

recoverFromLocalShards ? RecoveryState.Type.LOCAL_SHARDS : RecoveryState.Type.STORE, localNode, localNode);
if (recoverFromLocalShards) {
final List<IndexShard> startedShards = new ArrayList<>();
IndexMetaData sourceIndexMetaData = indicesService.getIndexMetaData(mergeSourceIndex);

This comment has been minimized.

Copy link
@bleskes

bleskes May 27, 2016

Member

Copying over from the previous iteration - can we get the metaData from IndexService.getIndexSettings? This will mean IndicesService doesn't need to proxy clusterService.state()

This comment has been minimized.

Copy link
@s1monw

s1monw May 27, 2016

Author Contributor

I really don't want to rely on the fact that we have the index allocated. I think if we can't find the index in the cluster state we should get the right exception which we won't if I get it through the index service

@bleskes

This comment has been minimized.

Copy link
Member

commented May 27, 2016

went through it again - looking good - life some very minor comments

s1monw added some commits May 27, 2016

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2016

@bleskes addressed your comments

s1monw added some commits May 27, 2016

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2016

I just ran this think for kicks to get some numbers:

Here I shrunk a 36GB 5 shards index into 1 shard WITHOUT the lucene hardlink directory optimization

index             shard time  type         stage source_host source_node target_host target_node repository snapshot files files_recovered files_percent files_total bytes       bytes_recovered bytes_percent bytes_total translog_ops translog_ops_recovered translog_ops_percent
wiki              0     156ms store        done  127.0.0.1   Dusk        127.0.0.1   Dusk        n/a        n/a      0     0               100.0%        13          0           0               100.0%        7680841604  0            0                      100.0%
wiki              1     1.3s  store        done  127.0.0.1   Dusk        127.0.0.1   Dusk        n/a        n/a      0     0               100.0%        13          0           0               100.0%        7678841298  0            0                      100.0%
wiki              2     848ms store        done  127.0.0.1   Dusk        127.0.0.1   Dusk        n/a        n/a      0     0               100.0%        13          0           0               100.0%        7644680461  0            0                      100.0%
wiki              3     1.2s  store        done  127.0.0.1   Dusk        127.0.0.1   Dusk        n/a        n/a      0     0               100.0%        13          0           0               100.0%        7673073968  0            0                      100.0%
wiki              4     1.1s  store        done  127.0.0.1   Dusk        127.0.0.1   Dusk        n/a        n/a      0     0               100.0%        13          0           0               100.0%        7697787879  0            0                      100.0%
wiki_single_shard 0     9.3m  local_shards done  127.0.0.1   Dusk        127.0.0.1   Dusk        n/a        n/a      60    60              100.0%        60          38375224070 38375224070     100.0%        38375224070 -1           0                      -1.0%

Here I shrunk a 36GB 5 shards index into 1 shard WITH the lucene hardlink directory optimization

index             shard time  type         stage source_host source_node target_host target_node repository snapshot files files_recovered files_percent files_total bytes bytes_recovered bytes_percent bytes_total translog_ops translog_ops_recovered translog_ops_percent
wiki              0     378ms store        done  127.0.0.1   Carnivore   127.0.0.1   Carnivore   n/a        n/a      0     0               100.0%        13          0     0               100.0%        7680841604  0            0                      100.0%
wiki              1     223ms store        done  127.0.0.1   Carnivore   127.0.0.1   Carnivore   n/a        n/a      0     0               100.0%        13          0     0               100.0%        7678841298  0            0                      100.0%
wiki              2     860ms store        done  127.0.0.1   Carnivore   127.0.0.1   Carnivore   n/a        n/a      0     0               100.0%        13          0     0               100.0%        7644680461  0            0                      100.0%
wiki              3     693ms store        done  127.0.0.1   Carnivore   127.0.0.1   Carnivore   n/a        n/a      0     0               100.0%        13          0     0               100.0%        7673073968  0            0                      100.0%
wiki              4     548ms store        done  127.0.0.1   Carnivore   127.0.0.1   Carnivore   n/a        n/a      0     0               100.0%        13          0     0               100.0%        7697787879  0            0                      100.0%
wiki_single_shard 0     710ms local_shards done  127.0.0.1   Carnivore   127.0.0.1   Carnivore   n/a        n/a      0     0               100.0%        60          0     0               100.0%        38375224070 -1           0                      -1.0%

copying all the files took ~10 min on a slow disk, the hardlink optimization took ~700ms which is essentially instant!

final List<IndexShard> startedShards = new ArrayList<>();
// look up if the index actually exists - we throw the right exception below instead of failing with ISE
final IndexMetaData sourceIndexMetaData = indicesService.getIndexMetaData(mergeSourceIndex);
final IndexService sourceIndexService = indicesService.indexService(mergeSourceIndex);

This comment has been minimized.

Copy link
@bleskes

bleskes May 30, 2016

Member

sorry, I still miss something. Why do we need to extend the interface of IndicesService with getIndexMetaData, which coming directly from the cluster state (introducing potential mismatches an entangles things more). Can't we use indicesService.getIndexServiceSafe(mergeSourceIndex) and get the same right exception? from there we can do indexService.getIndexSettings().getIndexMetaData() ?

This comment has been minimized.

Copy link
@s1monw

s1monw May 30, 2016

Author Contributor
  1. we don't want to throw that exception - we want to pass it to the listener
  2. we want only throw the exception if the index is not existing (not if it's just not allocated on this node)
  3. there is no mismatch since we use UUID
@bleskes

This comment has been minimized.

Copy link
Member

commented May 30, 2016

Looks great. Left a response on the little discussion around IndicesService,

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2016

since @bleskes asked I ran a _forcemerge with codec:"best_compression" on the new index and it's basically a 20% disk space reduction:

curl -XGET localhost:9200/_cat/segments?v
index             shard prirep ip        segment generation docs.count docs.deleted   size size.memory committed searchable version compound
wiki              0     p      127.0.0.1 _8a8         10736    3249095            0  7.1gb     7230889 true      true       5.4.1   false
wiki              1     p      127.0.0.1 _8dh         10853    3248161            0  7.1gb     7302713 true      true       5.4.1   false
wiki              2     p      127.0.0.1 _8bc         10776    3246424            0  7.1gb     7181590 true      true       5.4.1   false
wiki              3     p      127.0.0.1 _88u         10686    3246699            0  7.1gb     7359849 true      true       5.4.1   false
wiki              4     p      127.0.0.1 _8dr         10863    3244730            0  7.1gb     7428584 true      true       5.4.1   false
wiki_single_shard 0     p      127.0.0.1 _5               5   16235109            0 29.2gb    21294296 true      true       6.0.0   false

The .fdt file (stored fields) take 16.1 GB vs. 9.7GB with best_compression so basically all gains are on stored fields. ie. having 5 shards here on this wikipedia index has small overhead in terms of term dicts etc.

@@ -1129,8 +1129,4 @@ public void onRemoval(RemovalNotification<IndicesRequestCache.Key, IndicesReques
(Index index, IndexSettings indexSettings) -> canDeleteIndexContents(index, indexSettings);
private final IndexDeletionAllowedPredicate ALWAYS_TRUE = (Index index, IndexSettings indexSettings) -> true;

public IndexMetaData getIndexMetaData(Index index) {

This comment has been minimized.

Copy link
@bleskes

bleskes May 30, 2016

Member

Thank you!

This comment has been minimized.

Copy link
@s1monw

s1monw May 30, 2016

Author Contributor

it will cause false positives but I don't feel strong enough to block this for such a nit pick

@bleskes

This comment has been minimized.

Copy link
Member

commented May 30, 2016

LGTM. Thanks for the last commit.

s1monw added some commits May 30, 2016

@s1monw s1monw merged commit 502a775 into elastic:master May 31, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@s1monw s1monw removed the review label May 31, 2016

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.