[Remote Store] Inject remote store in IndexShard instead of RemoteStoreRefreshListener

[sachinpkale]

Signed-off-by: Sachin Kale [email protected]

Description

• Currently, we inject RemoteStoreRefreshListener into IndexShard where IndexService has the logic of creating an instance of RemoteStoreRefreshListener. As IndexShard already takes Store as one of the parameters, to make the behavior consistent, this change injects another instance of Store (created with RemoteDisrectory) to IndexShard.
• As per the comment [Remote Store] Upload segments to remote store post refresh #3460 (comment), RepositoriesService is injected into RemoteDirectoryFactory and factory takes care of fetching the repository for the given name.

Issues Resolved

• [Remote Store] Inject the RemoteStoreRefreshListener in the indexShard constructor #3548

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[opensearch-ci-bot]

✅   Gradle Check success 8a3c14c5fe347910bcc5938b69d01352464e7263
Log 6338

Reports 6338

[opensearch-ci-bot]

✅   Gradle Check success 7b3a31d510b5dd02ddfd5c6244ca3e139826e92a
Log 6343

Reports 6343

[Bukhtawar]

Maybe extract it into remoteStoreEnabled()

[sachinpkale]

Done

[Bukhtawar]

I'm not sure if the Store should be abstracted as a CompositeStore that has data backed in remote. IndexShard shouldn't worry about exposing or maintaining another Store that is remote

class CompositeStore extends Store
{ 
  private Store localStore;
  private Store remoteStore, 
}

[sachinpkale]

Need some time to think about this abstraction. Tracking here: #3719

[opensearch-ci-bot]

✅   Gradle Check success 49412c44fd45b4c154f66620c5e2c7b2ee1c0c88
Log 6383

Reports 6383

[Bukhtawar]

Sorry I couldn't find where are we handling the reference counting for Store. The call to directory() requires the Store to be Open(ref counter > 0) so this call would fail?

OpenSearch/server/src/main/java/org/opensearch/index/store/Store.java

Lines 208 to 211 in 3d4d5ca

	public Directory directory() {
	ensureOpen();
	return directory;
	}

[sachinpkale]

When store is created, default refCount is 1.

OpenSearch/libs/core/src/main/java/org/opensearch/common/util/concurrent/AbstractRefCounted.java

Lines 37 to 42 in 3d4d5ca

	/**
	* A basic RefCounted implementation that is initialized with a
	* ref count of 1 and calls {@link #closeInternal()} once it reaches
	* a 0 ref count
	*/
	public abstract class AbstractRefCounted implements RefCounted {

[Bukhtawar]

Thanks @sachinpkale so when should Store be closed for remote store as in reference count decremented or explicitly closed? I see IndexShard#closeShard closing the local store, do we need to handle close for remote store as well?

[sachinpkale]

As of now, we are not closing remoteStore. Let me add remoteStore close as a part of IndexShard.close()

[Bukhtawar]

For local store we have ShardStoreDeleter as a listener that cleans up data, for remote we are deleting it as a part of close. Also when a shard relocates to another node, the shard engine I believe closes, would that mean we are removing data from the remote? I guess that would be no, but please verify if that could happen

[Bukhtawar]

I just have a feeling we might be leaking the Store on abrupt Engine failures?

OpenSearch/server/src/main/java/org/opensearch/index/engine/InternalEngine.java

Lines 2231 to 2273 in 492ffc0

	/**
	* Closes the engine without acquiring the write lock. This should only be
	* called while the write lock is hold or in a disaster condition ie. if the engine
	* is failed.
	*/
	@Override
	protected final void closeNoLock(String reason, CountDownLatch closedLatch) {
	if (isClosed.compareAndSet(false, true)) {
	assert rwl.isWriteLockedByCurrentThread() || failEngineLock.isHeldByCurrentThread()
	: "Either the write lock must be held or the engine must be currently be failing itself";
	try {
	this.versionMap.clear();
	if (internalReaderManager != null) {
	internalReaderManager.removeListener(versionMap);
	}
	try {
	IOUtils.close(externalReaderManager, internalReaderManager);
	} catch (Exception e) {
	logger.warn("Failed to close ReaderManager", e);
	}
	try {
	IOUtils.close(translogManager.getTranslog());
	} catch (Exception e) {
	logger.warn("Failed to close translog", e);
	}
	// no need to commit in this case!, we snapshot before we close the shard, so translog and all sync'ed
	logger.trace("rollback indexWriter");
	try {
	indexWriter.rollback();
	} catch (AlreadyClosedException ex) {
	failOnTragicEvent(ex);
	throw ex;
	}
	logger.trace("rollback indexWriter done");
	} catch (Exception e) {
	logger.warn("failed to rollback writer on close", e);
	} finally {
	try {
	store.decRef();
	logger.debug("engine closed [{}]", reason);
	} finally {
	closedLatch.countDown();
	}

Can we add assertions in the tests to ensure the state of the remote store?

[sachinpkale]

Yes, we should not delete data on close. I think I have removed the blobcontainer.delete() as a part of some other commit which is not included in this PR. Let me add that.

I did not get the store leaking problem though. Are you talking about store or remoteStore?

[Bukhtawar]

In a case of abrupt Engine failure should we close the remote store similar to how a local store gets closed?

[sachinpkale]

But internal engine is not aware of the remoteStore. Its scope is limited to IndexShard.

[sachinpkale]

Build is failing with following exception:

* What went wrong:
Execution failed for task ':plugins:discovery-ec2:qa:amazon-ec2:yamlRestTestInstanceProfile'.
> `node{:plugins:discovery-ec2:qa:amazon-ec2:yamlRestTestInstanceProfile-0}` failed to wait for ports files after 135000 MILLISECONDS

Running on local worked fine.