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

Memoize isOnRemoteNode in index settings and refactor as well #12994

Merged
merged 2 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,7 @@ public static IndexMergePolicy fromString(String text) {

private volatile String defaultSearchPipeline;
private final boolean widenIndexSortType;
private final boolean isOnRemoteNode;

/**
* The maximum age of a retention lease before it is considered expired.
Expand Down Expand Up @@ -984,6 +985,7 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
* Now this sortField (IndexSort) is stored in SegmentInfo and we need to maintain backward compatibility for them.
*/
widenIndexSortType = IndexMetadata.SETTING_INDEX_VERSION_CREATED.get(settings).before(V_2_7_0);
isOnRemoteNode = RemoteStoreNodeAttribute.isRemoteDataAttributePresent(this.getNodeSettings());
gbbafna marked this conversation as resolved.
Show resolved Hide resolved

setEnableFuzzySetForDocId(scopedSettings.get(INDEX_DOC_ID_FUZZY_SET_ENABLED_SETTING));
setDocIdFuzzySetFalsePositiveProbability(scopedSettings.get(INDEX_DOC_ID_FUZZY_SET_FALSE_POSITIVE_PROBABILITY_SETTING));
Expand Down Expand Up @@ -1229,7 +1231,7 @@ public int getNumberOfReplicas() {
* proper index setting during the migration.
*/
public boolean isSegRepEnabledOrRemoteNode() {
return ReplicationType.SEGMENT.equals(replicationType) || isRemoteNode();
return ReplicationType.SEGMENT.equals(replicationType) || isOnRemoteNode();
}

public boolean isSegRepLocalEnabled() {
Expand All @@ -1247,8 +1249,8 @@ public boolean isRemoteStoreEnabled() {
return isRemoteStoreEnabled;
}

public boolean isRemoteNode() {
return RemoteStoreNodeAttribute.isRemoteDataAttributePresent(this.getNodeSettings());
public boolean isOnRemoteNode() {
return isOnRemoteNode;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ protected final void closeNoLock(String reason, CountDownLatch closedLatch) {
during promotion.
*/
if (engineConfig.getIndexSettings().isRemoteStoreEnabled() == false
&& engineConfig.getIndexSettings().isRemoteNode() == false) {
&& engineConfig.getIndexSettings().isOnRemoteNode() == false) {
latestSegmentInfos.counter = latestSegmentInfos.counter + SI_COUNTER_INCREMENT;
latestSegmentInfos.changed();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public RemoteStoreStatsTrackerFactory(ClusterService clusterService, Settings se

@Override
public void afterIndexShardCreated(IndexShard indexShard) {
if (indexShard.indexSettings().isRemoteStoreEnabled() == false && indexShard.indexSettings().isRemoteNode() == false) {
if (indexShard.indexSettings().isRemoteStoreEnabled() == false && indexShard.indexSettings().isOnRemoteNode() == false) {
return;
}
ShardId shardId = indexShard.shardId();
Expand Down
26 changes: 13 additions & 13 deletions server/src/main/java/org/opensearch/index/shard/IndexShard.java
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ public IndexShard(
logger,
threadPool,
this::getEngine,
indexSettings.isRemoteNode(),
indexSettings.isOnRemoteNode(),
() -> getRemoteTranslogUploadBufferInterval(remoteStoreSettings::getClusterRemoteTranslogBufferInterval)
);
this.mapperService = mapperService;
Expand Down Expand Up @@ -1469,7 +1469,7 @@ public SegmentsStats segmentStats(boolean includeSegmentFileSizes, boolean inclu
SegmentsStats segmentsStats = getEngine().segmentsStats(includeSegmentFileSizes, includeUnloadedSegments);
segmentsStats.addBitsetMemoryInBytes(shardBitsetFilterCache.getMemorySizeInBytes());
// Populate remote_store stats only if the index is remote store backed
if (indexSettings().isRemoteNode()) {
if (indexSettings().isOnRemoteNode()) {
segmentsStats.addRemoteSegmentStats(
new RemoteSegmentStats(remoteStoreStatsTrackerFactory.getRemoteSegmentTransferTracker(shardId).stats())
);
Expand All @@ -1491,7 +1491,7 @@ public FieldDataStats fieldDataStats(String... fields) {
public TranslogStats translogStats() {
TranslogStats translogStats = getEngine().translogManager().getTranslogStats();
// Populate remote_store stats only if the index is remote store backed
if (indexSettings.isRemoteNode()) {
if (indexSettings.isOnRemoteNode()) {
translogStats.addRemoteTranslogStats(
new RemoteTranslogStats(remoteStoreStatsTrackerFactory.getRemoteTranslogTransferTracker(shardId).stats())
);
Expand Down Expand Up @@ -1530,7 +1530,7 @@ public void flush(FlushRequest request) {
* {@link org.opensearch.index.translog.TranslogDeletionPolicy} for details
*/
public void trimTranslog() {
if (indexSettings.isRemoteNode()) {
if (indexSettings.isOnRemoteNode()) {
return;
}
verifyNotClosed();
Expand Down Expand Up @@ -2050,7 +2050,7 @@ public void close(String reason, boolean flushEngine, boolean deleted) throws IO
ToDo : Fix this https://github.com/opensearch-project/OpenSearch/issues/8003
*/
public RemoteSegmentStoreDirectory getRemoteDirectory() {
assert indexSettings.isRemoteNode();
assert indexSettings.isOnRemoteNode();
assert remoteStore.directory() instanceof FilterDirectory : "Store.directory is not an instance of FilterDirectory";
FilterDirectory remoteStoreDirectory = (FilterDirectory) remoteStore.directory();
FilterDirectory byteSizeCachingStoreDirectory = (FilterDirectory) remoteStoreDirectory.getDelegate();
Expand All @@ -2063,7 +2063,7 @@ public RemoteSegmentStoreDirectory getRemoteDirectory() {
* is in sync with local
*/
public boolean isRemoteSegmentStoreInSync() {
assert indexSettings.isRemoteNode();
assert indexSettings.isOnRemoteNode();
try {
RemoteSegmentStoreDirectory directory = getRemoteDirectory();
if (directory.readLatestMetadataFile() != null) {
Expand Down Expand Up @@ -2102,7 +2102,7 @@ public void waitForRemoteStoreSync() {
Calls onProgress on seeing an increased file count on remote
*/
public void waitForRemoteStoreSync(Runnable onProgress) {
assert indexSettings.isRemoteNode();
assert indexSettings.isOnRemoteNode();
RemoteSegmentStoreDirectory directory = getRemoteDirectory();
int segmentUploadeCount = 0;
if (shardRouting.primary() == false) {
Expand Down Expand Up @@ -2277,7 +2277,7 @@ public long recoverLocallyAndFetchStartSeqNo(boolean localTranslog) {
* @return the starting sequence number from which the recovery should start.
*/
private long recoverLocallyUptoLastCommit() {
assert indexSettings.isRemoteNode() : "Remote translog store is not enabled";
assert indexSettings.isOnRemoteNode() : "Remote translog store is not enabled";
long seqNo;
validateLocalRecoveryState();

Expand Down Expand Up @@ -3540,7 +3540,7 @@ assert getLocalCheckpoint() == primaryContext.getCheckpointStates().get(allocati
}

private void postActivatePrimaryMode() {
if (indexSettings.isRemoteNode()) {
if (indexSettings.isOnRemoteNode()) {
// We make sure to upload translog (even if it does not contain any operations) to remote translog.
// This helps to get a consistent state in remote store where both remote segment store and remote
// translog contains data.
Expand Down Expand Up @@ -4010,7 +4010,7 @@ public boolean enableUploadToRemoteTranslog() {
}

private boolean hasOneRemoteSegmentSyncHappened() {
assert indexSettings.isRemoteNode();
assert indexSettings.isOnRemoteNode();
// We upload remote translog only after one remote segment upload in case of migration
RemoteSegmentStoreDirectory rd = getRemoteDirectory();
AtomicBoolean segment_n_uploaded = new AtomicBoolean(false);
Expand Down Expand Up @@ -4624,7 +4624,7 @@ public final boolean isSearchIdle() {
public final boolean isSearchIdleSupported() {
// If the index is remote store backed, then search idle is not supported. This is to ensure that async refresh
// task continues to upload to remote store periodically.
if (isRemoteTranslogEnabled() || indexSettings.isRemoteNode()) {
if (isRemoteTranslogEnabled() || indexSettings.isOnRemoteNode()) {
return false;
}
return indexSettings.isSegRepEnabledOrRemoteNode() == false || indexSettings.getNumberOfReplicas() == 0;
Expand Down Expand Up @@ -5263,9 +5263,9 @@ enum ShardMigrationState {
}

static ShardMigrationState getShardMigrationState(IndexSettings indexSettings, boolean shouldSeed) {
if (indexSettings.isRemoteNode() && indexSettings.isRemoteStoreEnabled()) {
if (indexSettings.isOnRemoteNode() && indexSettings.isRemoteStoreEnabled()) {
return REMOTE_NON_MIGRATING;
} else if (indexSettings.isRemoteNode()) {
} else if (indexSettings.isOnRemoteNode()) {
return shouldSeed ? REMOTE_MIGRATING_UNSEEDED : REMOTE_MIGRATING_SEEDED;
}
return ShardMigrationState.DOCREP_NON_MIGRATING;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -893,7 +893,7 @@ public void beforeClose() {
* @throws IOException when there is an IO error committing.
*/
public void commitSegmentInfos(SegmentInfos latestSegmentInfos, long maxSeqNo, long processedCheckpoint) throws IOException {
assert indexSettings.isSegRepEnabledOrRemoteNode() || indexSettings.isRemoteNode();
assert indexSettings.isSegRepEnabledOrRemoteNode() || indexSettings.isOnRemoteNode();
metadataLock.writeLock().lock();
try {
final Map<String, String> userData = new HashMap<>(latestSegmentInfos.getUserData());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ TranslogWriter createWriter(
tragedy,
persistedSequenceNumberConsumer,
bigArrays,
indexSettings.isRemoteNode()
indexSettings.isOnRemoteNode()
);
} catch (final IOException e) {
throw new TranslogException(shardId, "failed to create new translog file", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -933,7 +933,7 @@ private EngineFactory getEngineFactory(final IndexSettings idxSettings) {
if (idxSettings.isRemoteSnapshot()) {
return config -> new ReadOnlyEngine(config, new SeqNoStats(0, 0, 0), new TranslogStats(), true, Function.identity(), false);
}
if (idxSettings.isSegRepEnabledOrRemoteNode() || idxSettings.isRemoteNode()) {
if (idxSettings.isSegRepEnabledOrRemoteNode() || idxSettings.isOnRemoteNode()) {
return new NRTReplicationEngineFactory();
}
return new InternalEngineFactory();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ private void doRecovery(final long recoveryId, final StartRecoveryRequest preExi
}
}
final boolean hasRemoteTranslog = recoveryTarget.state().getPrimary() == false
&& indexShard.indexSettings().isRemoteNode();
&& indexShard.indexSettings().isOnRemoteNode();
final boolean hasNoTranslog = indexShard.indexSettings().isRemoteSnapshot();
final boolean verifyTranslog = (hasRemoteTranslog || hasNoTranslog || hasRemoteSegmentStore) == false;
final long startingSeqNo = indexShard.recoverLocallyAndFetchStartSeqNo(!hasRemoteTranslog);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1053,4 +1053,15 @@ public void testDefaultSearchPipeline() throws Exception {
settings.updateIndexMetadata(metadata);
assertEquals("foo", settings.getDefaultSearchPipeline());
}

public void testIsOnRemoteNode() {
Version version = VersionUtils.getPreviousVersion();
Settings theSettings = Settings.builder()
.put(IndexMetadata.SETTING_VERSION_CREATED, version)
.put(IndexMetadata.SETTING_INDEX_UUID, "0xdeadbeef")
.build();
Settings nodeSettings = Settings.builder().put("node.attr.remote_store.translog.repository", "my-repo-1").build();
IndexSettings settings = newIndexSettings(newIndexMeta("index", theSettings), nodeSettings);
assertTrue("Index should be on remote node", settings.isOnRemoteNode());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ protected IndexShard newShard(
RemoteStoreStatsTrackerFactory remoteStoreStatsTrackerFactory = null;
RepositoriesService mockRepoSvc = mock(RepositoriesService.class);

if (indexSettings.isRemoteStoreEnabled() || indexSettings.isRemoteNode()) {
if (indexSettings.isRemoteStoreEnabled() || indexSettings.isOnRemoteNode()) {
String remoteStoreRepository = indexSettings.getRemoteStoreRepository();
// remote path via setting a repository . This is a hack used for shards are created using reset .
// since we can't get remote path from IndexShard directly, we are using repository to store it .
Expand Down Expand Up @@ -1498,7 +1498,7 @@ private SegmentReplicationTargetService prepareForReplication(

SegmentReplicationSourceFactory sourceFactory = null;
SegmentReplicationTargetService targetService;
if (primaryShard.indexSettings.isRemoteStoreEnabled() || primaryShard.indexSettings.isRemoteNode()) {
if (primaryShard.indexSettings.isRemoteStoreEnabled() || primaryShard.indexSettings.isOnRemoteNode()) {
RecoverySettings recoverySettings = new RecoverySettings(
Settings.EMPTY,
new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)
Expand Down
Loading