Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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 @@ -43,6 +43,8 @@ public class DefaultRecreateHandler implements RecreateIndexHandler {
private static final String TRANSLOG = "translog";
private static final String DURABILITY = "durability";
private static final String SYNC_INTERVAL = "sync_interval";
private static final String REFRESH_DISABLED = "-1";
private static final String DEFAULT_LIVE_REFRESH_INTERVAL = "1s";

private EventPublisherJob jobData;

Expand Down Expand Up @@ -676,13 +678,33 @@ static String buildRevertJson(IndexSettings live, BulkIndexOverrides bulk) {
}

private static String pickRefreshInterval(IndexSettings live, BulkIndexOverrides bulk) {
String refreshInterval = null;
if (live != null && live.getRefreshInterval() != null) {
return live.getRefreshInterval();
refreshInterval = live.getRefreshInterval();
} else if (bulk != null && bulk.getRefreshInterval() != null) {
refreshInterval = DEFAULT_LIVE_REFRESH_INTERVAL; // bulk disabled refresh; restore default
}
if (bulk != null && bulk.getRefreshInterval() != null) {
return "1s"; // bulk disabled refresh; restore near-real-time default
}
return null;
if (isRefreshDisabled(refreshInterval)) {
LOG.warn(
"Configured live refresh_interval '{}' disables refresh and would leave the promoted "
+ "index unsearchable until a manual _refresh. Overriding to '{}' so documents stay "
+ "searchable after promotion.",
refreshInterval,
DEFAULT_LIVE_REFRESH_INTERVAL);
refreshInterval = DEFAULT_LIVE_REFRESH_INTERVAL;
}
return refreshInterval;
}

/**
* A {@code refresh_interval} of {@code -1} disables refresh entirely. That is correct for a
* staged index nothing reads from, but as a <em>live</em> serving value it leaves newly indexed
* documents invisible to search until a manual {@code _refresh} — the "reindex finishes but the
* page is empty" symptom. The live-revert must never apply it, regardless of what the admin
* configured on {@code liveIndexSettings} or what fell back from the bulk overrides.
*/
private static boolean isRefreshDisabled(String refreshInterval) {
return refreshInterval != null && REFRESH_DISABLED.equals(refreshInterval.trim());
}

private static Integer pickReplicas(IndexSettings live, BulkIndexOverrides bulk) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1457,6 +1457,24 @@ void partialLiveOverridesBulk() {
assertTrue(json.contains("\"sync_interval\":\"5s\""));
}

@Test
@DisplayName("Live refresh_interval '-1' is never applied as a live value (guard -> 1s)")
void liveRefreshDisabledIsOverriddenToDefault() {
// Misconfiguration: liveIndexSettings.refreshInterval is "-1" (refresh disabled) — e.g.
// copied from the bulk side or a stale saved config. Without the guard the revert would
// faithfully re-apply "-1" to the promoted index, leaving it unsearchable until a manual
// _refresh (the "reindex finishes but the page is empty" symptom). The revert must override
// it back to the near-real-time default.
org.openmetadata.schema.system.IndexSettings live =
new org.openmetadata.schema.system.IndexSettings().withRefreshInterval("-1");
org.openmetadata.schema.system.BulkIndexOverrides bulk =
new org.openmetadata.schema.system.BulkIndexOverrides().withRefreshInterval("-1");
String json = DefaultRecreateHandler.buildRevertJson(live, bulk);
assertNotNull(json);
assertTrue(json.contains("\"refresh_interval\":\"1s\""));
assertFalse(json.contains("\"refresh_interval\":\"-1\""));
}
Comment on lines +1460 to +1476

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test doesn't isolate the primary bug scenario

The new test passes -1 for both live and bulk, but because the live branch wins (first if in pickRefreshInterval), bulk's value never influences the result. The scenario where the misconfiguration exists on live alone (live="-1", bulk=null) — i.e., no bulk-reindex was active — is the most direct form of the bug and goes untested. A separate parameterized case for bulk=null would pin that path and prevent a future refactor from inadvertently skipping the guard only when bulk is absent.


@Test
@DisplayName("Bulk JSON properly escapes admin-supplied string values")
void bulkSettingsEscapesQuotesInValues() {
Expand Down
Loading