Adding CheckpointRefreshListener to trigger when Segment replication is turned on and Primary shard refreshes

[Rishikesh1159]

Description

This PR will allow us to add checkpointRefreshListener when primary shard refreshes and publish the checkpoint action. This will be used for segment replication, as when a refresh happens on primary shard, the checkpoint action will be published to all the replicas. The logic of what to do on a replica shard when a new checkpoint is received is not part of this PR, that will be added later in another PR.

This is a part of the process of merging our feature branch - feature/segment-replication - back into main by re-PRing our changes from the feature branch.
The breakdown of the plan to merge to main is detailed here: #2355
For added context on segment replication - here's the design proposal #2229

Issues Resolved

#2927

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 failure 37f49d2
Log 4868

Reports 4868

[mch2]

I understand that this will eventually be tested with integration tests when we are actually copying & refreshing replicas. I'm thinking we should also test this refresh logic in isolation by:

1. Creating a primary that wires up the new refresh listener.
2. Refresh he primary
3. Assert the replica has received the new nrt checkpoint.

[mch2]

I'm thinking we should overload the constructor here (or subclass indexShard) and conditionally use it if segrep is enabled when IndexShards are created from IndexService.

[Bukhtawar]

The logic for creating an IndexShard should take of injecting the correct concrete implementation or noop as needed

[mch2]

This isn't used anywhere? It should be wired up in newEngineConfig below.

Should this be an internal or external listener?

[Rishikesh1159]

Yes, I will wire this up. But not sure if it should be external or internal listener. I am using internal listener for now, we can have a discussion on this.

[Bukhtawar]

Lets push this down to the creation of IndexShard so that we can minimize the branching across the board and create the right flavour of IndexShard for segrep

[mch2]

I liked this idea at first, but I think we should maybe leave this block while the feature is still behind feature flags. Once the feature flag is removed then we wire up the listener even if its a NoOp.

[kartg]

Nitpick - can be written as:

List<ReferenceManager.RefreshListener> internalRefreshListeners = Arrays.asList(new RefreshMetricUpdater(refreshMetric));
if (this. checkpointRefreshListener != null) {
    internalRefreshListeners.add(checkpointRefreshListener);
}
internalRefreshListeners = Collections.unmodifiableList(internalRefreshListeners);

[Rishikesh1159]

@kartg this we how we have it now, The above highlighted one is outdated.

final List<ReferenceManager.RefreshListener> internalRefreshListener;
        if (this.checkpointRefreshListener != null) {
            internalRefreshListener = Arrays.asList(new RefreshMetricUpdater(refreshMetric), checkpointRefreshListener);
        } else {
            internalRefreshListener = Collections.singletonList(new RefreshMetricUpdater(refreshMetric));
        }

Also if we try to write it as your suggested way, then internalRefreshListeners cannot be final, we have been using it as final even before this PR. Not sure if changing internalRefreshListener to a non-final variable is a good idea.

[kartg]

Given that this block declares and defines the internalRefreshListener variable, the final modifier doesn't add value IMO. What is more valuable to maintain is the unmodifiable list, which is lost if checkpointRefreshListener is non-null.

If you'd really like to retain the final modifier, I'd suggest doing something like this:

List<ReferenceManager.RefreshListener> refreshListenerList = Arrays.asList(new RefreshMetricUpdater(refreshMetric));
if (this. checkpointRefreshListener != null) {
    refreshListenerList.add(checkpointRefreshListener);
}
final List<ReferenceManager.RefreshListener> internalRefreshListener = Collections.unmodifiableList(refreshListenerList);

[Rishikesh1159]

@kartg I tried to incorporate your code block, but it throws me UnsupportedOperation Exception when we are adding, "refreshListenerList.add(checkpointRefreshListener);". As we are using Arrays.asList(new RefreshMetricUpdater(refreshMetric)); we cannot use .add method. Instead we have to again convert this into actual List. More about this exception can be found here. As this is a nitpick I am just using the way how it was before to avoid complexity

[opensearch-ci-bot]

❌   Gradle Check failure 3210e33
Log 5036

Reports 5036

[opensearch-ci-bot]

❌   Gradle Check failure b216116
Log 5037

Reports 5037

[Rishikesh1159]

start gradle check

[opensearch-ci-bot]

✅   Gradle Check success b216116
Log 5041

Reports 5041

[opensearch-ci-bot]

❌   Gradle Check failure 4928270
Log 5095

Reports 5095

[opensearch-ci-bot]

❌   Gradle Check failure 7777d1e
Log 5096

Reports 5096

[opensearch-ci-bot]

✅   Gradle Check success 5907f0b
Log 5099

Reports 5099

[opensearch-ci-bot]

❌   Gradle Check failure e40dc41
Log 5107

Reports 5107

[opensearch-ci-bot]

✅   Gradle Check success 5a9d544
Log 5185

Reports 5185

[opensearch-ci-bot]

✅   Gradle Check success 9bbfe20
Log 5319

Reports 5319

[mch2]

Some small things but overall lgtm. Thanks @Rishikesh1159!

@Bukhtawar Any concerns here with this under the feature flag?

[opensearch-ci-bot]

✅   Gradle Check success 4c97019
Log 5482

Reports 5482

[kartg]

Nitpick - can be written as:

List<ReferenceManager.RefreshListener> internalRefreshListeners = Arrays.asList(new RefreshMetricUpdater(refreshMetric));
if (this. checkpointRefreshListener != null) {
    internalRefreshListeners.add(checkpointRefreshListener);
}
internalRefreshListeners = Collections.unmodifiableList(internalRefreshListeners);

[kartg]

Should this be gated by the feature flag too? Alternatively, should indexSettings.isSegRepEnabled() check the feature flag internally?

[mch2]

• 1 I think this should check the feature flag internally in a separate change.

[Rishikesh1159]

As @mch2 said we will do this in a separate change not in this PR

[opensearch-ci-bot]

✅   Gradle Check success bd17e4f
Log 5546

Reports 5546

[opensearch-ci-bot]

✅   Gradle Check success 6c75f6d
Log 5548

Reports 5548

[opensearch-ci-bot]

✅   Gradle Check success d78e8ae
Log 5549

Reports 5549

[kartg]

Nitpick - javadoc needs update

[Rishikesh1159]

Sorry, thanks for catching this. Will fix this next commit.

[kartg]

Nitpick - why do you need this refresh call if you're simply testing the list of internal refresh listeners below?

[Rishikesh1159]

Yes this might not be needed for this test case, as we are only testing if listener is added or not. Will remove this.

[opensearch-ci-bot]

✅   Gradle Check success 591e608
Log 5551

Reports 5551