From 9904d1f5ae4e1d4c4ac9bba69e211c805cd87f9c Mon Sep 17 00:00:00 2001 From: Shantanu Tiwari Date: Tue, 8 Sep 2026 20:29:10 +0530 Subject: [PATCH 01/10] Update Snapshot workflow --- .github/workflows/maven-central-snapshot.yml | 23 +++++++------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/.github/workflows/maven-central-snapshot.yml b/.github/workflows/maven-central-snapshot.yml index e102a8b..7142d88 100644 --- a/.github/workflows/maven-central-snapshot.yml +++ b/.github/workflows/maven-central-snapshot.yml @@ -15,6 +15,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + - name: Set up JDK 17 and Maven Central credentials uses: actions/setup-java@v5 with: @@ -24,17 +25,7 @@ jobs: server-id: central server-username: MAVEN_USERNAME server-password: MAVEN_PASSWORD - gpg-private-key: ${{ secrets.CENTRAL_GPG_KEY }} - gpg-passphrase: MAVEN_GPG_PASSPHRASE - - name: Verify GPG key and passphrase - shell: bash - env: - GPG_KEY: ${{ secrets.CENTRAL_GPG_KEY }} - GPG_PASSPHRASE: ${{ secrets.CENTRAL_GPG_KEY_PASSWORD }} - run: | - test -n "$GPG_KEY" || { echo "CENTRAL_GPG_KEY is unavailable"; exit 1; } - test -n "$GPG_PASSPHRASE" || { echo "CENTRAL_GPG_KEY_PASSWORD is unavailable"; exit 1; } - gpg --batch --list-secret-keys --keyid-format LONG + - name: Set snapshot version from project version shell: bash run: | @@ -44,15 +35,17 @@ jobs: else snapshot_version="${project_version}-SNAPSHOT" fi + echo "Publishing ${snapshot_version}" mvn --batch-mode versions:set \ -DnewVersion="$snapshot_version" \ -DprocessAllModules=true \ -DgenerateBackupPoms=false - - name: Verify snapshot build + + - name: Build and test run: mvn --batch-mode verify + - name: Publish snapshot - run: mvn --batch-mode -P release deploy -DskipTests + run: mvn --batch-mode deploy -DskipTests env: MAVEN_USERNAME: ${{ secrets.MAVEN_CENTRAL_TOKEN_USERNAME }} - MAVEN_PASSWORD: ${{ secrets.MAVEN_CENTRAL_TOKEN_PASSWORD }} - MAVEN_GPG_PASSPHRASE: ${{ secrets.CENTRAL_GPG_KEY_PASSWORD }} + MAVEN_PASSWORD: ${{ secrets.MAVEN_CENTRAL_TOKEN_PASSWORD }} \ No newline at end of file From e945684dc5d8d89aea7c69cd6a2ec416f3720de0 Mon Sep 17 00:00:00 2001 From: Shantanu Tiwari Date: Wed, 9 Sep 2026 22:15:58 +0530 Subject: [PATCH 02/10] Add delivery-time fire history and expose it on the dashboard --- .gitignore | 2 + docs/docs/concepts/error-codes.md | 2 + .../java/com/phonepe/magazine/Magazine.java | 37 ++ .../magazine/core/BaseMagazineStorage.java | 29 ++ .../magazine/entity/FireCheckpoint.java | 35 ++ .../phonepe/magazine/exception/ErrorCode.java | 4 +- .../exception/MagazineExceptions.java | 4 + .../impl/aerospike/AerospikeStorage.java | 31 +- .../aerospike/AerospikeStorageConfig.java | 24 ++ .../aerospike/AerospikeStorageValidator.java | 21 + .../aerospike/common/AerospikeConstants.java | 17 + .../impl/aerospike/common/ErrorMessage.java | 8 + .../store/MagazineMetadataStore.java | 214 +++++++++- .../magazine/metrics/StorageOperation.java | 4 + .../aerospike/MagazineFireHistoryTest.java | 403 ++++++++++++++++++ .../magazine/resources/MagazineResource.java | 13 + .../response/FireHistoryResponse.java | 37 ++ .../magazine/response/FireHistorySpan.java | 37 ++ .../magazine/response/FireHistoryWindow.java | 45 ++ .../magazine/service/MagazineService.java | 97 +++++ .../magazineAssets/magazineIndex.html | 33 +- .../main/resources/magazineAssets/style.css | 8 + .../magazine/MagazineDashboardAssetsTest.java | 11 +- .../MagazineDashboardDemoApplication.java | 39 +- .../resources/MagazineResourceTest.java | 151 ++++++- 25 files changed, 1293 insertions(+), 13 deletions(-) create mode 100644 magazine-core/src/main/java/com/phonepe/magazine/entity/FireCheckpoint.java create mode 100644 magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java create mode 100644 magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistoryResponse.java create mode 100644 magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistorySpan.java create mode 100644 magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistoryWindow.java diff --git a/.gitignore b/.gitignore index f1b3da5..2049654 100644 --- a/.gitignore +++ b/.gitignore @@ -37,3 +37,5 @@ docs/site/ # Generated documentation site docs/site/ + +/magazine-benchmark/ diff --git a/docs/docs/concepts/error-codes.md b/docs/docs/concepts/error-codes.md index 41ffb5b..b3a24eb 100644 --- a/docs/docs/concepts/error-codes.md +++ b/docs/docs/concepts/error-codes.md @@ -17,6 +17,8 @@ exception and switch on the code; the message is for humans, the code is the con | `INVALID_SHARDS` | A shard count is out of range, or a shard-layout change was refused. | | `INVALID_CONFIGURATION` | A configuration value is missing or invalid. Raised at construction. | | `DATA_TYPE_MISMATCH` | The payload does not match the magazine's declared type, or deduplication was enabled for an unsupported type. | +| `NOT_ENABLED` | An optional capability was asked for but is switched off in configuration — currently only `firePointerBefore` without `fireHistoryEnabled`. Distinct from `NOT_IMPLEMENTED`: the capability exists, it is simply not turned on. | +| `INVALID_REQUEST` | The call itself was out of range for this magazine's current state, though the configuration is sound — for example asking `firePointerBefore` about a moment older than the retained fire history. The same magazine answers a more recent instant correctly. | ## `NOTHING_TO_FIRE` vs `RETRIES_EXHAUSTED` diff --git a/magazine-core/src/main/java/com/phonepe/magazine/Magazine.java b/magazine-core/src/main/java/com/phonepe/magazine/Magazine.java index c03d01a..6da15a4 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/Magazine.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/Magazine.java @@ -17,10 +17,13 @@ package com.phonepe.magazine; import com.phonepe.magazine.core.BaseMagazineStorage; +import com.phonepe.magazine.entity.FireCheckpoint; import com.phonepe.magazine.entity.MagazineContext; import com.phonepe.magazine.entity.MagazineData; import com.phonepe.magazine.entity.MetaData; import com.phonepe.magazine.exception.MagazineExceptions; +import java.time.Instant; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -117,6 +120,40 @@ public int getShards() { return context.getShards(); } + /** + * Where each shard's fire pointer stood at {@code instant}. + *

+ * Slots are claimed in order, so one number per shard answers "which slots had been handed out + * by then". Every slot at or below a shard's returned pointer + * was claimed at or before {@code instant}. + *

+ * Requires {@code fireHistoryEnabled} on the storage configuration. + * + * @param instant The moment to report on. + * @return shard id to checkpoint, keyed as {@link #getMetaData()} is. A shard is absent when no + * checkpoint reaches back that far yet, which is normal for a young magazine and means + * the caller should do nothing this round rather than assume the shard is empty. + * @throws com.phonepe.magazine.exception.MagazineException {@code NOT_ENABLED} when fire + * history is disabled; {@code INVALID_REQUEST} when history was retained but has + * already been evicted past {@code instant}, which means the configured window is too + * narrow for the age being asked about. + */ + public Map firePointerBefore(final Instant instant) { + return baseMagazineStorage.firePointerBefore(context, instant); + } + + /** + * Every checkpoint currently retained, newest first. + * + * @return shard id to its checkpoints, newest first, keyed as {@link #getMetaData()} is. A + * shard is absent when it has never recorded one. + * @throws com.phonepe.magazine.exception.MagazineException {@code NOT_ENABLED} when fire + * history is disabled. + */ + public Map> fireHistory() { + return baseMagazineStorage.fireHistory(context); + } + /** * Peek data from specific shards and pointers within the magazine. * diff --git a/magazine-core/src/main/java/com/phonepe/magazine/core/BaseMagazineStorage.java b/magazine-core/src/main/java/com/phonepe/magazine/core/BaseMagazineStorage.java index bd97794..b85cedd 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/core/BaseMagazineStorage.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/core/BaseMagazineStorage.java @@ -16,6 +16,7 @@ package com.phonepe.magazine.core; +import com.phonepe.magazine.entity.FireCheckpoint; import com.phonepe.magazine.entity.MagazineContext; import com.phonepe.magazine.entity.MagazineData; import com.phonepe.magazine.entity.MagazineScope; @@ -23,6 +24,8 @@ import com.phonepe.magazine.entity.StorageType; import com.phonepe.magazine.exception.ErrorCode; import com.phonepe.magazine.exception.MagazineExceptions; +import java.time.Instant; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -141,6 +144,32 @@ public abstract Set> peek( final Map> shardPointersMap ); + /** + * Where each shard's fire pointer stood at a given moment. + *

+ * Optional. Backends that cannot answer must raise {@code NOT_ENABLED} rather than guess. + * + * @param context The magazine operation context. + * @param instant The moment to report on. + * @return shard id to checkpoint, keyed as {@link #getMetaData} is. A shard is absent when + * nothing is known about it that far back, which is normal for a young magazine. + */ + public abstract Map firePointerBefore( + final MagazineContext context, + final Instant instant + ); + + /** + * Every retained checkpoint, newest first. A pure read: unlike {@link #firePointerBefore} it + * records nothing. + *

+ * Optional. Backends that cannot answer must raise {@code NOT_ENABLED} rather than guess. + * + * @param context The magazine operation context. + * @return shard id to its checkpoints, newest first, keyed as {@link #getMetaData} is. + */ + public abstract Map> fireHistory(final MagazineContext context); + private static void validate(final StorageType type, final int recordTtl, final int metaDataTtl, diff --git a/magazine-core/src/main/java/com/phonepe/magazine/entity/FireCheckpoint.java b/magazine-core/src/main/java/com/phonepe/magazine/entity/FireCheckpoint.java new file mode 100644 index 0000000..f34acbb --- /dev/null +++ b/magazine-core/src/main/java/com/phonepe/magazine/entity/FireCheckpoint.java @@ -0,0 +1,35 @@ +/** + * Copyright (c) 2025 Original Author(s), PhonePe India Pvt. Ltd. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.phonepe.magazine.entity; + +import java.time.Instant; + +/** + * Where a shard's fire pointer stood at a known moment. + *

+ * The guarantee is one-directional and that is the whole point: every slot at or below + * {@link #firePointer()} was claimed at or before {@link #recordedAt()}. It may under-report - the + * pointer was possibly further along by then - but it can never over-report. A caller deciding + * whether a record has been abandoned can therefore act on it without ever mistaking a live + * delivery for a dead one. + * + * @param firePointer the highest slot known to have been claimed by {@code recordedAt}. + * @param recordedAt when that observation was taken. Exposed rather than hidden so callers can + * tell how far behind the answer is and alert on it. + */ +public record FireCheckpoint(long firePointer, Instant recordedAt) { +} diff --git a/magazine-core/src/main/java/com/phonepe/magazine/exception/ErrorCode.java b/magazine-core/src/main/java/com/phonepe/magazine/exception/ErrorCode.java index c36fd4f..a1d5afe 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/exception/ErrorCode.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/exception/ErrorCode.java @@ -26,5 +26,7 @@ public enum ErrorCode { NOT_IMPLEMENTED, INVALID_SHARDS, INVALID_CONFIGURATION, - DATA_TYPE_MISMATCH + DATA_TYPE_MISMATCH, + NOT_ENABLED, + INVALID_REQUEST } diff --git a/magazine-core/src/main/java/com/phonepe/magazine/exception/MagazineExceptions.java b/magazine-core/src/main/java/com/phonepe/magazine/exception/MagazineExceptions.java index b553490..6765b60 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/exception/MagazineExceptions.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/exception/MagazineExceptions.java @@ -30,6 +30,10 @@ public static MagazineException invalidConfiguration(final String message) { return of(ErrorCode.INVALID_CONFIGURATION, message); } + public static MagazineException invalidRequest(final String message) { + return of(ErrorCode.INVALID_REQUEST, message); + } + public static MagazineException invalidShards(final String message) { return of(ErrorCode.INVALID_SHARDS, message); } diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java index 587169b..39f629c 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java @@ -25,6 +25,7 @@ import com.phonepe.magazine.entity.MetaData; import com.phonepe.magazine.entity.StorageType; import com.phonepe.magazine.exception.ErrorCode; +import com.phonepe.magazine.entity.FireCheckpoint; import com.phonepe.magazine.exception.MagazineException; import com.phonepe.magazine.exception.MagazineExceptions; import com.phonepe.magazine.impl.aerospike.common.AerospikeConstants; @@ -39,6 +40,8 @@ import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.Metrics; import io.micrometer.core.instrument.composite.CompositeMeterRegistry; +import java.time.Instant; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.OptionalLong; @@ -101,7 +104,8 @@ public AerospikeStorage( this.initializer = new AerospikeMagazineInitializer(aerospikeClient, retryerFactory, namespace, metaSetName, storageConfig.getShards(), storageConfig.isAllowShardIncrease()); this.metadataStore = new MagazineMetadataStore(aerospikeClient, retryerFactory, metrics, - namespace, metaSetName, getMetaDataTtl()); + namespace, metaSetName, getMetaDataTtl(), storageConfig.isFireHistoryEnabled(), + storageConfig.fireHistoryWindowMillis(), storageConfig.getFireHistoryEntries()); this.dataStore = new MagazineDataStore<>(aerospikeClient, retryerFactory, metrics, namespace, dataSetName, getRecordTtl(), clazz); this.activeShards = new ActiveShardSelector(this::loadActiveShards, @@ -226,6 +230,31 @@ public void delete(final MagazineContext context, final MagazineData magazine } } + @Override + public Map firePointerBefore(final MagazineContext context, final Instant instant) { + if (Objects.isNull(instant)) { + throw MagazineExceptions.invalidRequest("Instant is required."); + } + try { + return metadataStore.firePointerBefore(context, instant); + } catch (MagazineException e) { + throw e; + } catch (Exception e) { + throw mapFailure(e, ErrorMessage.ERROR_GETTING_META_DATA, context); + } + } + + @Override + public Map> fireHistory(final MagazineContext context) { + try { + return metadataStore.fireHistory(context); + } catch (MagazineException e) { + throw e; + } catch (Exception e) { + throw mapFailure(e, ErrorMessage.ERROR_GETTING_META_DATA, context); + } + } + @Override public Set> peek(final MagazineContext context, final Map> shardPointersMap) { diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorageConfig.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorageConfig.java index c54088f..9ee0d7d 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorageConfig.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorageConfig.java @@ -78,4 +78,28 @@ public class AerospikeStorageConfig { */ @Builder.Default private boolean metricsEnabled = true; + /** + * Whether to record delivery-time checkpoints, which is what {@code firePointerBefore} reads. + * Off by default: the checkpoint map rides the pointer record, and every claim rewrites that + * record, so a caller that never asks should not pay. + */ + @Builder.Default + private boolean fireHistoryEnabled = false; + /** + * Seconds covered by one checkpoint, and so the resolution of every answer + * {@code firePointerBefore} can give. One entry per shard per window, whatever the throughput. + */ + @Builder.Default + private int fireHistoryWindowSeconds = AerospikeConstants.DEFAULT_FIRE_HISTORY_WINDOW_SECONDS; + /** + * Checkpoints retained per shard. Eviction is by count rather than age, so an intermittently + * used magazine reaches further back than {@code window x entries} instead of having its + * history wiped by the first write after an idle period. + */ + @Builder.Default + private int fireHistoryEntries = AerospikeConstants.DEFAULT_FIRE_HISTORY_ENTRIES; + + public long fireHistoryWindowMillis() { + return fireHistoryWindowSeconds * 1_000L; + } } diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorageValidator.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorageValidator.java index 284e432..faab6a8 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorageValidator.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorageValidator.java @@ -47,9 +47,30 @@ static AerospikeStorageConfig validateConfig(final AerospikeStorageConfig storag throw MagazineExceptions.invalidConfiguration( "Active shard refresh seconds must be at least 1."); } + validateFireHistory(storageConfig); return storageConfig; } + /** + * The checkpoint map rides the pointer record, which every claim rewrites, so both bounds exist + * to keep that record small. + */ + private static void validateFireHistory(final AerospikeStorageConfig storageConfig) { + if (!storageConfig.isFireHistoryEnabled()) { + return; + } + if (storageConfig.getFireHistoryWindowSeconds() < 1) { + throw MagazineExceptions.invalidConfiguration( + "Fire history window seconds must be at least 1."); + } + if (storageConfig.getFireHistoryEntries() < 2 + || storageConfig.getFireHistoryEntries() > AerospikeConstants.MAX_FIRE_HISTORY_ENTRIES) { + throw MagazineExceptions.invalidConfiguration(String.format( + "Fire history entries must be between 2 and %d.", + AerospikeConstants.MAX_FIRE_HISTORY_ENTRIES)); + } + } + static void validateStorage(final IAerospikeClient aerospikeClient, final Class clazz, final boolean enableDeDupe) { diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikeConstants.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikeConstants.java index 58216b3..b195abe 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikeConstants.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikeConstants.java @@ -41,15 +41,26 @@ public final class AerospikeConstants { public static final String METADATA_SCHEMA_VERSION = "META_VERSION"; public static final String CREATED_AT = "CREATED_AT"; public static final String SHARDS_BIN = "SHARDS"; + /** + * Delivery-time checkpoints: a key-ordered map of {@code negated window start -> fire pointer}. + * Keys are negated so the map sorts newest-first, which makes eviction a single index-range + * removal that keeps the newest N entries. + */ + public static final String FIRE_HISTORY = "FIRE_HISTORY"; /** * Bins to project on metadata batch reads. Metadata records are read {@code shards}-wide on * every active-shard refresh, so projecting keeps that fan-out from carrying bins nobody reads. + * {@link #FIRE_HISTORY} is deliberately absent: only {@code firePointerBefore} consults it. */ @Getter private static final String[] metadataBins = { LOAD_POINTER, FIRE_POINTER, LOAD_COUNTER, FIRE_COUNTER}; + /** Bins to project when reading delivery-time checkpoints. */ + @Getter + private static final String[] fireHistoryBins = {FIRE_POINTER, FIRE_HISTORY}; + /** Bins to project on data reads - the payload is the only bin any caller consumes. */ @Getter private static final String[] dataBins = {DATA}; @@ -105,4 +116,10 @@ public final class AerospikeConstants { public static final int DEFAULT_SHARDS = 8; public static final int SHARD_CONFIGURATION_TTL_SECONDS = 5 * 365 * 24 * 60 * 60; + + // --- fire history ----------------------------------------------------------------------- + public static final int DEFAULT_FIRE_HISTORY_WINDOW_SECONDS = 300; + /** Small on purpose: the map rides the pointer record, and every claim rewrites that record. */ + public static final int DEFAULT_FIRE_HISTORY_ENTRIES = 32; + public static final int MAX_FIRE_HISTORY_ENTRIES = 128; } diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/ErrorMessage.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/ErrorMessage.java index ba9bdc1..4f2f11d 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/ErrorMessage.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/ErrorMessage.java @@ -35,6 +35,14 @@ public final class ErrorMessage { public static final String ERROR_INITIALIZING_MAGAZINE = "Error initializing magazine [magazineIdentifier = %s]"; public static final String ERROR_PEEKING_DATA = "Error peeking data [magazineIdentifier = %s]"; + public static final String FIRE_HISTORY_DISABLED = + "Fire history is not enabled for this storage [magazineIdentifier = %s]. Set " + + "fireHistoryEnabled on the storage configuration to record delivery-time " + + "checkpoints."; + public static final String FIRE_HISTORY_TOO_SHORT = + "Fire history does not reach back to %3$s [magazineIdentifier = %1$s, shard = %2$d]. The " + + "oldest retained checkpoint is %4$s; widen fireHistoryWindowSeconds or raise " + + "fireHistoryEntries."; public static final String MISSING_METADATA_RECORD = "Metadata record is missing [magazineIdentifier = %s, shard = %d]. Metadata TTL must " + "outlive record TTL; this indicates the magazine metadata expired or was deleted."; diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java index 08797bb..3826894 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java @@ -23,11 +23,19 @@ import com.aerospike.client.Operation; import com.aerospike.client.Record; import com.aerospike.client.ResultCode; +import com.aerospike.client.Value; +import com.aerospike.client.cdt.MapOperation; +import com.aerospike.client.cdt.MapOrder; +import com.aerospike.client.cdt.MapPolicy; +import com.aerospike.client.cdt.MapReturnType; +import com.aerospike.client.cdt.MapWriteFlags; import com.aerospike.client.exp.Exp; import com.aerospike.client.policy.RecordExistsAction; import com.aerospike.client.policy.WritePolicy; +import com.phonepe.magazine.entity.FireCheckpoint; import com.phonepe.magazine.entity.MagazineContext; import com.phonepe.magazine.entity.MetaData; +import com.phonepe.magazine.exception.ErrorCode; import com.phonepe.magazine.exception.MagazineExceptions; import com.phonepe.magazine.impl.aerospike.common.AerospikeConstants; import com.phonepe.magazine.impl.aerospike.common.AerospikeNaming; @@ -36,12 +44,19 @@ import com.phonepe.magazine.impl.aerospike.common.ErrorMessage; import com.phonepe.magazine.metrics.MagazineMetrics; import com.phonepe.magazine.metrics.StorageOperation; +import java.time.Instant; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.OptionalLong; +import java.util.SortedMap; +import java.util.TreeMap; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicLong; +import lombok.extern.slf4j.Slf4j; /** * Reads and writes the per-shard metadata records: load/fire pointers and load/fire counters. @@ -55,8 +70,16 @@ * copy, and {@link Key} construction runs a RIPEMD-160 digest. Rebuilding those per operation was * pure overhead on every round trip. */ +@Slf4j public final class MagazineMetadataStore { + /** + * {@code CREATE_ONLY | NO_FAIL} makes a second writer in the same window a server-side no-op + * rather than an error, which is what lets every process attempt the write independently. + */ + private static final MapPolicy CHECKPOINT_MAP_POLICY = new MapPolicy( + MapOrder.KEY_ORDERED, MapWriteFlags.CREATE_ONLY | MapWriteFlags.NO_FAIL); + /** * The claim's precondition: this shard still has an unconsumed slot. Absent bins read as zero, * so a record never fired from does not make the comparison unknown. @@ -74,20 +97,41 @@ public final class MagazineMetadataStore { private final WritePolicy claimPolicy; private final WritePolicy adjustPolicy; + private final WritePolicy checkpointPolicy; + private final boolean fireHistoryEnabled; + private final long fireHistoryWindowMillis; + private final int fireHistoryEntries; /** Bounded by the number of configured magazines. */ private final Map keyCache = new ConcurrentHashMap<>(); + /** + * Last checkpoint window written, per shard, so only one caller per window issues each write. + * Per shard rather than per magazine: a shard skipped because its record did not exist yet, or + * because its write failed, must be retried within the same window rather than sit out until + * the next one. + *

+ * Keyed by context like {@link #keyCache}, because one store serves every magazine on a + * storage. Shard count is part of the context's equality, so the array can never outlive the + * shard count it was sized for. + */ + private final Map checkpointWindows = new ConcurrentHashMap<>(); public MagazineMetadataStore(final IAerospikeClient client, final AerospikeRetryer retryerFactory, final MagazineMetrics metrics, final String namespace, final String metaSetName, - final int metaDataTtl) { + final int metaDataTtl, + final boolean fireHistoryEnabled, + final long fireHistoryWindowMillis, + final int fireHistoryEntries) { this.client = client; this.retryerFactory = retryerFactory; this.metrics = metrics; this.namespace = namespace; this.metaSetName = metaSetName; + this.fireHistoryEnabled = fireHistoryEnabled; + this.fireHistoryWindowMillis = fireHistoryWindowMillis; + this.fireHistoryEntries = fireHistoryEntries; this.claimPolicy = AerospikePolicies.writePolicy(client); this.claimPolicy.recordExistsAction = RecordExistsAction.UPDATE_ONLY; @@ -99,6 +143,12 @@ public MagazineMetadataStore(final IAerospikeClient client, this.adjustPolicy = AerospikePolicies.writePolicy(client); this.adjustPolicy.recordExistsAction = RecordExistsAction.UPDATE; this.adjustPolicy.expiration = metaDataTtl; + + // UPDATE_ONLY: a checkpoint describes a shard that exists. Creating the record here would + // fabricate a pointer record for a shard that was never initialised. + this.checkpointPolicy = AerospikePolicies.writePolicy(client); + this.checkpointPolicy.recordExistsAction = RecordExistsAction.UPDATE_ONLY; + this.checkpointPolicy.expiration = metaDataTtl; } /** @@ -214,6 +264,10 @@ public int[] activeShards(final MagazineContext context) { ? pointerRecords : batchRead(context, keys(context).counterKeys(), StorageOperation.REFRESH_ACTIVE_SHARDS); + // The batch read above already holds every shard's fire pointer, so recording checkpoints + // here costs no additional read, and this cache refreshes asynchronously. + recordFireHistory(context, pointerRecords); + final int[] active = new int[context.getShards()]; int found = 0; for (int shard = 0; shard < context.getShards(); shard++) { @@ -226,7 +280,163 @@ public int[] activeShards(final MagazineContext context) { active[found++] = shard; } } - return found == active.length ? active : java.util.Arrays.copyOf(active, found); + return found == active.length ? active : Arrays.copyOf(active, found); + } + + /** + * Where each shard's fire pointer stood at {@code instant}. + *

+ * Answers come from checkpoints recorded at or before {@code instant}, so a returned pointer is + * a lower bound: every slot at or below it was claimed by then. It may lag reality, never lead + * it. + * + * @return shard id to checkpoint. A shard is absent when no checkpoint is old enough yet. + * @throws com.phonepe.magazine.exception.MagazineException {@code NOT_ENABLED} when fire + * history is disabled, {@code INVALID_REQUEST} when retained history does not reach + * back to {@code instant}. + */ + public Map firePointerBefore(final MagazineContext context, final Instant instant) { + if (!fireHistoryEnabled) { + throw MagazineExceptions.of(ErrorCode.NOT_ENABLED, String.format( + ErrorMessage.FIRE_HISTORY_DISABLED, context.getMagazineIdentifier())); + } + final Record[] records = readFireHistory(context); + + // Recorded before resolving, so a shard whose history cannot answer still advances: the + // throw below must not stop this magazine from ever accumulating usable history again. + recordFireHistory(context, records); + + final Map checkpoints = new HashMap<>(capacityFor(context.getShards())); + for (int shard = 0; shard < context.getShards(); shard++) { + final int current = shard; + resolveCheckpoint(context, records[current], instant, current) + .ifPresent(checkpoint -> checkpoints.put(context.shardId(current), checkpoint)); + } + return checkpoints; + } + + /** + * Every retained checkpoint, newest first + */ + public Map> fireHistory(final MagazineContext context) { + if (!fireHistoryEnabled) { + throw MagazineExceptions.of(ErrorCode.NOT_ENABLED, String.format( + ErrorMessage.FIRE_HISTORY_DISABLED, context.getMagazineIdentifier())); + } + final Record[] records = readFireHistory(context); + final Map> history = new HashMap<>(capacityFor(context.getShards())); + for (int shard = 0; shard < context.getShards(); shard++) { + final List checkpoints = readHistory(records[shard]).entrySet().stream() + .map(entry -> new FireCheckpoint(entry.getValue(), Instant.ofEpochMilli(-entry.getKey()))) + .toList(); + if (!checkpoints.isEmpty()) { + history.put(context.shardId(shard), checkpoints); + } + } + return history; + } + + private Record[] readFireHistory(final MagazineContext context) { + return retryerFactory.call(() -> { + metrics.aerospikeCall(context.getMagazineIdentifier(), StorageOperation.READ_FIRE_HISTORY); + return client.get(client.getBatchPolicyDefault(), + keys(context).pointerKeys().toArray(new Key[0]), + AerospikeConstants.getFireHistoryBins()); + }); + } + + private Optional resolveCheckpoint(final MagazineContext context, + final Record record, + final Instant instant, + final int shard) { + final SortedMap history = readHistory(record); + final long cutoff = instant.toEpochMilli(); + for (Map.Entry entry : history.entrySet()) { + final long windowStart = -entry.getKey(); + if (windowStart <= cutoff) { + return Optional.of(new FireCheckpoint(entry.getValue(), Instant.ofEpochMilli(windowStart))); + } + } + // At capacity with nothing old enough means history that could have answered was evicted. + // Returning empty would be indistinguishable from "nothing to do" and would silently stop + // the caller from ever acting. + if (history.size() >= fireHistoryEntries) { + throw MagazineExceptions.invalidRequest(String.format(ErrorMessage.FIRE_HISTORY_TOO_SHORT, + context.getMagazineIdentifier(), shard, instant, + Instant.ofEpochMilli(-history.lastKey()))); + } + return Optional.empty(); + } + + @SuppressWarnings("unchecked") + private SortedMap readHistory(final Record record) { + final Object raw = Objects.isNull(record) ? null : record.getValue(AerospikeConstants.FIRE_HISTORY); + return raw instanceof Map ? new TreeMap<>((Map) raw) : new TreeMap<>(); + } + + /** + * Records one checkpoint per shard, at most once per window per process. + *

+ * Deliberately best-effort: a checkpoint improves the reader's precision but is never a + * correctness requirement, since a missed window leaves the previous checkpoint in place and + * that still states something true. Failing a cache refresh or a read because a checkpoint + * could not be written would turn a cosmetic loss into an outage. + *

+ * Claimed per shard, and the claim is released again if the write fails, so a shard that had no + * record yet or whose write errored is retried on the next call rather than sitting the window + * out. + */ + private void recordFireHistory(final MagazineContext context, final Record[] pointerRecords) { + if (!fireHistoryEnabled) { + return; + } + final long window = System.currentTimeMillis() / fireHistoryWindowMillis * fireHistoryWindowMillis; + final AtomicLong[] claims = checkpointWindows.computeIfAbsent(context, this::newClaims); + final List pointerKeys = keys(context).pointerKeys(); + // Negated so the key-ordered map sorts newest-first and eviction is a single index range. + final Value windowKey = Value.get(-window); + + for (int shard = 0; shard < context.getShards(); shard++) { + final Record pointers = pointerRecords[shard]; + if (Objects.isNull(pointers) || !claim(claims[shard], window)) { + continue; + } + try { + writeCheckpoint(context, pointerKeys.get(shard), windowKey, + pointers.getLong(AerospikeConstants.FIRE_POINTER)); + } catch (Exception e) { + claims[shard].compareAndSet(window, Long.MIN_VALUE); + log.warn("Could not record fire history for magazine {} shard {} window {}.", + context.getMagazineIdentifier(), shard, window, e); + } + } + } + + private void writeCheckpoint(final MagazineContext context, final Key key, final Value windowKey, + final long firePointer) { + metrics.aerospikeCall(context.getMagazineIdentifier(), StorageOperation.WRITE_FIRE_HISTORY); + client.operate(checkpointPolicy, key, + // CREATE_ONLY|NO_FAIL: the first writer in a window wins and every other process is + // a server-side no-op. Every writer's value is a lower bound, so whichever wins + // recorded the most conservative pointer observed for that window. + MapOperation.put(CHECKPOINT_MAP_POLICY, AerospikeConstants.FIRE_HISTORY, + windowKey, Value.get(firePointer)), + // Keep the newest N by count, not by age: an age cutoff computed from the clock + // would discard the whole map on the first write after an idle period longer than + // the span, blinding readers even though those checkpoints were still usable. + MapOperation.removeByIndexRange(AerospikeConstants.FIRE_HISTORY, + fireHistoryEntries, MapReturnType.NONE)); + } + + private static boolean claim(final AtomicLong claimed, final long window) { + final long previous = claimed.get(); + return previous < window && claimed.compareAndSet(previous, window); + } + + private AtomicLong[] newClaims(final MagazineContext context) { + final AtomicLong[] claims = new AtomicLong[context.getShards()]; + Arrays.setAll(claims, index -> new AtomicLong(Long.MIN_VALUE)); + return claims; } /** diff --git a/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java b/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java index 3f36d62..c09ed40 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java @@ -51,6 +51,10 @@ public enum StorageOperation { WRITE_DATA("write_data"), DELETE_DATA("delete_data"), BATCH_READ_DATA("batch_read_data"), + /** The checkpoint write; one per shard per window, not one per claim. */ + WRITE_FIRE_HISTORY("write_fire_history"), + /** The batch read behind {@code firePointerBefore}. */ + READ_FIRE_HISTORY("read_fire_history"), CLAIM_DEDUPE_MARKER("claim_dedupe_marker"), WITHDRAW_DEDUPE_MARKER("withdraw_dedupe_marker"); diff --git a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java new file mode 100644 index 0000000..d26e7f6 --- /dev/null +++ b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java @@ -0,0 +1,403 @@ +/** + * Copyright (c) 2025 Original Author(s), PhonePe India Pvt. Ltd. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.phonepe.magazine.impl.aerospike; + +import com.aerospike.client.Key; +import com.aerospike.client.Record; +import com.phonepe.magazine.Magazine; +import com.phonepe.magazine.entity.FireCheckpoint; +import com.phonepe.magazine.entity.MagazineScope; +import com.phonepe.magazine.exception.ErrorCode; +import com.phonepe.magazine.exception.MagazineException; +import com.phonepe.magazine.impl.aerospike.common.AerospikeConstants; +import org.awaitility.Awaitility; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.time.Duration; +import java.time.Instant; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.TreeMap; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Delivery-time checkpoints and {@link Magazine#firePointerBefore(Instant)}. + *

+ * The property under test throughout is one-directional: a reported pointer must be one the shard + * had certainly reached by the reported time. Under-reporting is fine and expected; over-reporting + * would let a caller take a record away from a consumer that is still working on it. + *

+ * Windows are one second wide here so rollover is observable without the tests taking minutes. + */ +class MagazineFireHistoryTest extends AerospikeMagazineTestBase { + + private static final String NAMESPACE = "NAMESPACE"; + private static final String META_SET = "FARM_ID_META_SET"; + private static final int WINDOW_SECONDS = 1; + + @Test + @DisplayName("fire history is off unless asked for, and asking it anything then fails loudly") + void disabledByDefault() { + final Magazine magazine = magazine("HIST_DISABLED", "HIST_DISABLED_SET", false, 4, 8); + magazine.load("one"); + magazine.fire(); + + assertMagazineError(ErrorCode.NOT_ENABLED, () -> magazine.firePointerBefore(Instant.now())); + assertNull(history(magazine, 0), "no checkpoint bin should exist when the feature is off"); + } + + @Test + @DisplayName("one checkpoint per shard per window, however many times it is asked for") + void recordsOneCheckpointPerWindow() { + final int shards = 4; + // A wide window so the whole test runs inside one, making "same window" unambiguous. + final Magazine magazine = magazine("HIST_ONE", "HIST_ONE_SET", true, shards, 8, 60); + loadAndFire(magazine, 3); + + // Shards only get a metadata record once something is loaded to them, and shard choice is + // random, so the expectation is one checkpoint per shard that exists - not per configured shard. + final int withRecords = shardsWithMetadata(magazine, shards); + assertTrue(withRecords > 0, "at least one shard should have been written to"); + + magazine.firePointerBefore(Instant.now()); + assertEquals(withRecords, totalCheckpoints(magazine, shards), + "one window should record exactly one checkpoint on each shard that exists"); + + magazine.firePointerBefore(Instant.now()); + magazine.firePointerBefore(Instant.now()); + assertEquals(withRecords, totalCheckpoints(magazine, shards), + "further calls inside the same window must not add entries"); + } + + @Test + @DisplayName("a new window adds a checkpoint without disturbing the old one") + void newWindowAddsACheckpoint() throws Exception { + final Magazine magazine = magazine("HIST_ROLL", "HIST_ROLL_SET", true, 1, 8); + loadAndFire(magazine, 2); + magazine.firePointerBefore(Instant.now()); + final Map first = new TreeMap<>(history(magazine, 0)); + + Thread.sleep(1_100); + loadAndFire(magazine, 2); + magazine.firePointerBefore(Instant.now()); + + // Counted as "more than before" rather than "exactly two": the active-shard refresh records + // on its own schedule, so the number of windows a test crosses is not something it controls. + final Map second = new TreeMap<>(history(magazine, 0)); + assertTrue(second.size() > first.size(), "a later window should have been appended"); + first.forEach((window, pointer) -> assertEquals(pointer, second.get(window), + "an existing checkpoint must never be rewritten")); + } + + @Test + @DisplayName("history is capped by count, so it cannot grow without bound") + void historyIsBoundedByCount() throws Exception { + final int cap = 3; + final Magazine magazine = magazine("HIST_CAP", "HIST_CAP_SET", true, 1, cap); + for (int round = 0; round < cap + 3; round++) { + loadAndFire(magazine, 1); + magazine.firePointerBefore(Instant.now()); + Thread.sleep(1_100); + } + assertEquals(cap, history(magazine, 0).size(), + "the newest entries are kept and everything past the cap is dropped"); + } + + @Test + @DisplayName("a recorded pointer never runs ahead of the shard's actual fire pointer") + void checkpointsNeverOverReport() { + final Magazine magazine = magazine("HIST_SAFE", "HIST_SAFE_SET", true, 1, 8); + loadAndFire(magazine, 5); + magazine.firePointerBefore(Instant.now()); + + final Record record = record(magazine, 0); + final long actual = record.getLong(AerospikeConstants.FIRE_POINTER); + history(magazine, 0).values().forEach(recorded -> assertTrue(recorded <= actual, + "checkpoint " + recorded + " must not exceed the fire pointer " + actual)); + } + + @Test + @DisplayName("the answer is the newest checkpoint at or before the instant asked about") + void returnsNewestCheckpointNotAfterTheInstant() throws Exception { + final Magazine magazine = magazine("HIST_PICK", "HIST_PICK_SET", true, 1, 8); + loadAndFire(magazine, 2); + magazine.firePointerBefore(Instant.now()); + final Instant afterFirstBurst = Instant.now(); + final long pointerAfterFirstBurst = record(magazine, 0).getLong(AerospikeConstants.FIRE_POINTER); + + Thread.sleep(1_100); + loadAndFire(magazine, 4); + magazine.firePointerBefore(Instant.now()); + + final FireCheckpoint checkpoint = magazine.firePointerBefore(afterFirstBurst).get("SHARD_0"); + assertNotNull(checkpoint, "the first burst's window is old enough to answer with"); + assertFalse(checkpoint.recordedAt().isAfter(afterFirstBurst), + "the answer must come from a window that had already closed by then"); + assertTrue(checkpoint.firePointer() <= pointerAfterFirstBurst, + "the second burst's pointer must not be used to answer an earlier question"); + } + + @Test + @DisplayName("a young magazine reports nothing rather than failing") + void youngHistoryReturnsAbsent() { + final Magazine magazine = magazine("HIST_YOUNG", "HIST_YOUNG_SET", true, 1, 8); + loadAndFire(magazine, 1); + + // Nothing has been recorded this far back, and the map is nowhere near capacity, so the + // magazine simply has not lived long enough. That is normal and must not be an error. + assertTrue(magazine.firePointerBefore(Instant.now().minus(Duration.ofHours(1))).isEmpty()); + } + + @Test + @DisplayName("a full history that cannot reach back far enough is a configuration error, not an empty answer") + void evictedHistoryFailsLoudly() throws Exception { + final int cap = 2; + final Magazine magazine = magazine("HIST_EVICTED", "HIST_EVICTED_SET", true, 1, cap); + final Instant longAgo = Instant.now().minus(Duration.ofHours(1)); + for (int round = 0; round < cap + 1; round++) { + loadAndFire(magazine, 1); + magazine.firePointerBefore(Instant.now()); + Thread.sleep(1_100); + } + + // At capacity with every retained entry newer than the question: real history was + // discarded. Returning empty would look identical to "nothing to do" and would silently + // stop a caller from ever acting. It is the instant that is out of range, not the config - + // the same magazine answers a recent instant perfectly well. + assertMagazineError(ErrorCode.INVALID_REQUEST, () -> magazine.firePointerBefore(longAgo)); + } + + @Test + @DisplayName("reading advances history, so a burst that is never fired from again stays reachable") + void readingRecordsACheckpoint() throws Exception { + final Magazine magazine = magazine("HIST_BURST", "HIST_BURST_SET", true, 1, 8); + loadAndFire(magazine, 3); + magazine.firePointerBefore(Instant.now()); + + Thread.sleep(1_100); + // No load, no fire: the magazine is idle. Reading alone must still move history forward, + // otherwise a magazine that took one burst and was then abandoned would hold a single + // checkpoint predating the burst and its orphans would never become visible. + magazine.firePointerBefore(Instant.now()); + + assertEquals(2, history(magazine, 0).size(), + "the read should have recorded the new window by itself"); + } + + @Test + @DisplayName("consuming records checkpoints on its own, without anyone reading them") + void firingRecordsCheckpoints() { + final Magazine magazine = magazine("HIST_FIRE", "HIST_FIRE_SET", true, 1, 8); + loadAndFire(magazine, 4); + + // The active-shard cache refreshes asynchronously, so this arrives shortly after the fire + // rather than during it. + Awaitility.await().atMost(Duration.ofSeconds(20)).pollInterval(Duration.ofMillis(200)) + .untilAsserted(() -> { + loadAndFire(magazine, 1); + assertFalse(history(magazine, 0).isEmpty(), + "firing alone should eventually record a checkpoint"); + }); + } + + @Test + @DisplayName("a shard that only appears mid-window still gets that window's checkpoint") + void lateShardIsCheckpointedInTheSameWindow() { + final int shards = 8; + // One window wide enough to hold the whole test, so nothing here can roll into the next. + final Magazine magazine = magazine("HIST_LATE", "HIST_LATE_SET", true, shards, 8, 60); + loadAndFire(magazine, 1); + magazine.firePointerBefore(Instant.now()); + final int firstPass = shardsWithMetadata(magazine, shards); + assertEquals(firstPass, totalCheckpoints(magazine, shards)); + + // Shard choice is random, so keep loading until a shard that did not exist during the first + // pass does now. + for (int attempt = 0; attempt < 50 && shardsWithMetadata(magazine, shards) == firstPass; attempt++) { + loadAndFire(magazine, 1); + } + final int secondPass = shardsWithMetadata(magazine, shards); + assertTrue(secondPass > firstPass, "expected a shard to appear after the first pass"); + + magazine.firePointerBefore(Instant.now()); + assertEquals(secondPass, totalCheckpoints(magazine, shards), + "a shard that appeared after the window was first written must still be recorded; " + + "claiming the window per magazine would leave it blank until the next one"); + } + + @Test + @DisplayName("the whole retained history reads back newest first, and reading it records nothing") + void fireHistoryReturnsEverythingRetainedWithoutRecording() throws Exception { + final Magazine magazine = magazine("HIST_ALL", "HIST_ALL_SET", true, 1, 8); + for (int round = 0; round < 3; round++) { + loadAndFire(magazine, 1); + magazine.firePointerBefore(Instant.now()); + Thread.sleep(1_100); + } + + final List checkpoints = magazine.fireHistory().get("SHARD_0"); + assertNotNull(checkpoints); + assertEquals(history(magazine, 0).size(), checkpoints.size(), "every retained entry is returned"); + for (int i = 1; i < checkpoints.size(); i++) { + assertTrue(checkpoints.get(i - 1).recordedAt().isAfter(checkpoints.get(i).recordedAt()), + "newest first"); + assertTrue(checkpoints.get(i - 1).firePointer() >= checkpoints.get(i).firePointer(), + "pointers only ever move forward"); + } + + // A console browsing history must not write to the magazine it is inspecting. + final int before = history(magazine, 0).size(); + Thread.sleep(1_100); + magazine.fireHistory(); + assertEquals(before, history(magazine, 0).size(), "reading history must not record a window"); + } + + @Test + @DisplayName("reading the whole history is refused when the feature is off") + void fireHistoryIsRefusedWhenDisabled() { + final Magazine magazine = magazine("HIST_ALL_OFF", "HIST_ALL_OFF_SET", false, 1, 8); + magazine.load("one"); + assertMagazineError(ErrorCode.NOT_ENABLED, magazine::fireHistory); + } + + @Test + @DisplayName("the checkpoint map stays off the hot metadata read path") + void checkpointsAreNotProjectedOntoMetadataReads() { + assertFalse(Arrays.asList(AerospikeConstants.getMetadataBins()).contains(AerospikeConstants.FIRE_HISTORY), + "metadata batch reads fan out per shard on every refresh; carrying the checkpoint " + + "map there would put it on the hottest read in the system"); + } + + @Test + @DisplayName("history sizing is validated at construction") + void configurationIsValidated() { + assertMagazineError(ErrorCode.INVALID_CONFIGURATION, + () -> storageWithHistory("BAD_WINDOW_SET", true, 0, 8)); + assertMagazineError(ErrorCode.INVALID_CONFIGURATION, + () -> storageWithHistory("BAD_ENTRIES_LOW_SET", true, 1, 1)); + assertMagazineError(ErrorCode.INVALID_CONFIGURATION, + () -> storageWithHistory("BAD_ENTRIES_HIGH_SET", true, 1, + AerospikeConstants.MAX_FIRE_HISTORY_ENTRIES + 1)); + } + + // --- helpers --------------------------------------------------------------------------- + + private Magazine magazine(final String identifier, final String setName, + final boolean enabled, final int shards, final int entries) { + return magazine(identifier, setName, enabled, shards, entries, WINDOW_SECONDS); + } + + private Magazine magazine(final String identifier, final String setName, + final boolean enabled, final int shards, final int entries, + final int windowSeconds) { + return Magazine.builder() + .magazineIdentifier(identifier) + .baseMagazineStorage(storageWithHistory(setName, enabled, windowSeconds, entries, shards)) + .build(); + } + + private AerospikeStorage storageWithHistory(final String setName, final boolean enabled, + final int windowSeconds, final int entries) { + return storageWithHistory(setName, enabled, windowSeconds, entries, 1); + } + + private AerospikeStorage storageWithHistory(final String setName, final boolean enabled, + final int windowSeconds, final int entries, + final int shards) { + return buildStorage(AerospikeStorageConfig.builder() + .namespace(NAMESPACE) + .dataSetName(setName) + .metaSetName("META_SET") + .recordTtl(30 * 24 * 60 * 60) + .metaDataTtl(2 * 30 * 24 * 60 * 60) + .shards(shards) + // Refresh often, so the fire path exercises the recorder within a test's life. + .activeShardRefreshSeconds(1) + .fireHistoryEnabled(enabled) + .fireHistoryWindowSeconds(windowSeconds) + .fireHistoryEntries(entries) + .build(), + String.class, false, "FARM_ID", "CLIENT_ID", MagazineScope.LOCAL, aerospikeClient); + } + + private void loadAndFire(final Magazine magazine, final int count) { + for (int i = 0; i < count; i++) { + magazine.load("message-" + random.nextLong()); + } + for (int i = 0; i < count; i++) { + fireWhenVisible(magazine); + } + } + + /** + * Draining a shard suppresses it from the active-shard cache, and the cache only reloads + * asynchronously on access. A load immediately after a drain is therefore not yet visible to + * fire(), which is expected behaviour rather than something these tests should assert on. + */ + private void fireWhenVisible(final Magazine magazine) { + Awaitility.await().atMost(Duration.ofSeconds(15)).pollInterval(Duration.ofMillis(100)) + .until(() -> { + try { + return magazine.fire() != null; + } catch (MagazineException e) { + if (e.getErrorCode() == ErrorCode.NOTHING_TO_FIRE) { + return false; + } + throw e; + } + }); + } + + private Record record(final Magazine magazine, final int shard) { + final String name = magazine.getShards() <= 1 + ? magazine.getMagazineIdentifier() + "_" + AerospikeConstants.METADATA + : magazine.getMagazineIdentifier() + "_SHARD_" + shard + "_" + AerospikeConstants.METADATA; + return aerospikeClient.get(aerospikeClient.getReadPolicyDefault(), new Key(NAMESPACE, META_SET, name)); + } + + @SuppressWarnings("unchecked") + private Map history(final Magazine magazine, final int shard) { + final Record record = record(magazine, shard); + if (record == null) { + return null; + } + final Object raw = record.getValue(AerospikeConstants.FIRE_HISTORY); + return raw instanceof Map ? (Map) raw : null; + } + + private int shardsWithMetadata(final Magazine magazine, final int shards) { + int found = 0; + for (int shard = 0; shard < shards; shard++) { + if (record(magazine, shard) != null) { + found++; + } + } + return found; + } + + private int totalCheckpoints(final Magazine magazine, final int shards) { + int total = 0; + for (int shard = 0; shard < shards; shard++) { + final Map history = history(magazine, shard); + total += history == null ? 0 : history.size(); + } + return total; + } +} diff --git a/magazine-dw-bundle/src/main/java/com/phonepe/magazine/resources/MagazineResource.java b/magazine-dw-bundle/src/main/java/com/phonepe/magazine/resources/MagazineResource.java index 1eec897..e783120 100644 --- a/magazine-dw-bundle/src/main/java/com/phonepe/magazine/resources/MagazineResource.java +++ b/magazine-dw-bundle/src/main/java/com/phonepe/magazine/resources/MagazineResource.java @@ -17,6 +17,7 @@ package com.phonepe.magazine.resources; import com.phonepe.magazine.request.PeekRequest; +import com.phonepe.magazine.response.FireHistoryResponse; import com.phonepe.magazine.response.MagazineDescriptor; import com.phonepe.magazine.response.MagazineMetadataResponse; import com.phonepe.magazine.response.PeekResponse; @@ -28,6 +29,7 @@ import jakarta.ws.rs.Path; import jakarta.ws.rs.PathParam; import jakarta.ws.rs.Produces; +import jakarta.ws.rs.QueryParam; import jakarta.ws.rs.core.MediaType; import java.util.List; import lombok.RequiredArgsConstructor; @@ -56,6 +58,17 @@ public MagazineMetadataResponse metadata(@PathParam("identifier") final String i return service.metadata(identifier); } + /** + * @param shard a single shard to report on, or absent for totals. Shard count is unbounded, so + * per-shard series are fetched one at a time rather than as a grid. + */ + @GET + @Path("/{identifier}/fire-history") + public FireHistoryResponse fireHistory(@PathParam("identifier") final String identifier, + @QueryParam("shard") final String shard) { + return service.fireHistory(identifier, shard); + } + @POST @Path("/{identifier}/peek") @Consumes(MediaType.APPLICATION_JSON) diff --git a/magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistoryResponse.java b/magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistoryResponse.java new file mode 100644 index 0000000..e68da9b --- /dev/null +++ b/magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistoryResponse.java @@ -0,0 +1,37 @@ +/** + * Copyright (c) 2025 Original Author(s), PhonePe India Pvt. Ltd. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + +package com.phonepe.magazine.response; + +import java.util.List; + +/** + * @param enabled false when the magazine does not record delivery-time checkpoints. Reported rather + * than raised: a disabled feature is a configuration fact, not a failure. + * @param shards every shard with recorded history, for choosing one to drill into. + * @param shard the shard {@code windows} describes, or null when they are totals across shards. + * @param span how far back history reaches, or null when nothing is recorded yet. + * @param windows retained windows, newest first. Bounded by {@code fireHistoryEntries}, and + * independent of shard count. + */ +public record FireHistoryResponse(String identifier, + boolean enabled, + List shards, + String shard, + FireHistorySpan span, + List windows) { +} diff --git a/magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistorySpan.java b/magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistorySpan.java new file mode 100644 index 0000000..cc1f486 --- /dev/null +++ b/magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistorySpan.java @@ -0,0 +1,37 @@ +/** + * Copyright (c) 2025 Original Author(s), PhonePe India Pvt. Ltd. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + +package com.phonepe.magazine.response; + +import com.fasterxml.jackson.annotation.JsonFormat; +import java.time.Instant; + +/** + * How far back the retained history actually reaches. + *

+ * The number an operator needs before trusting anything built on this: a redrive asking about a + * moment older than {@link #oldest()} gets an error rather than an answer. Reported from the data + * rather than from {@code window x entries}, because eviction is by count and gaps stretch the real + * span well beyond the nominal one. + */ +public record FireHistorySpan(@JsonFormat(shape = JsonFormat.Shape.STRING) Instant oldest, + @JsonFormat(shape = JsonFormat.Shape.STRING) Instant newest, + long spanSeconds, + int windows, + int shardsReporting, + int shardsConfigured) { +} diff --git a/magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistoryWindow.java b/magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistoryWindow.java new file mode 100644 index 0000000..5c029bd --- /dev/null +++ b/magazine-dw-bundle/src/main/java/com/phonepe/magazine/response/FireHistoryWindow.java @@ -0,0 +1,45 @@ +/** + * Copyright (c) 2025 Original Author(s), PhonePe India Pvt. Ltd. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + + +package com.phonepe.magazine.response; + +import com.fasterxml.jackson.annotation.JsonFormat; +import java.time.Instant; + +/** + * One checkpoint window, already reduced to numbers. + *

+ * Reduced server-side rather than shipped as a shard-to-pointer map because shard count is + * unbounded: a 256-shard magazine at the maximum 128 retained windows would otherwise be a 32,768 + * entry payload on a 30-second poll, and a 256-column table nobody can read. A window row is + * O(1) in shard count, so this scales to any magazine; one shard at a time is available by asking + * for it. + * + * @param shardsReporting shards that recorded this window. Below the configured count is normal - + * a shard has no metadata record until something is loaded to it. + * @param firePointer summed across reporting shards, or that shard's own pointer when one + * shard was asked for. + * @param delivered slots handed out since the next-older window, or null for the oldest row. + * Summed only over shards present in both windows: a shard that + * appeared in between would otherwise show its whole pointer as a delta. + */ +public record FireHistoryWindow(@JsonFormat(shape = JsonFormat.Shape.STRING) Instant recordedAt, + long ageSeconds, + int shardsReporting, + long firePointer, + Long delivered) { +} diff --git a/magazine-dw-bundle/src/main/java/com/phonepe/magazine/service/MagazineService.java b/magazine-dw-bundle/src/main/java/com/phonepe/magazine/service/MagazineService.java index 66d8a18..698fb81 100644 --- a/magazine-dw-bundle/src/main/java/com/phonepe/magazine/service/MagazineService.java +++ b/magazine-dw-bundle/src/main/java/com/phonepe/magazine/service/MagazineService.java @@ -21,9 +21,15 @@ import com.github.benmanes.caffeine.cache.LoadingCache; import com.phonepe.magazine.Magazine; import com.phonepe.magazine.MagazineManager; +import com.phonepe.magazine.entity.FireCheckpoint; import com.phonepe.magazine.entity.MagazineData; import com.phonepe.magazine.entity.MetaData; +import com.phonepe.magazine.exception.ErrorCode; +import com.phonepe.magazine.exception.MagazineException; import com.phonepe.magazine.request.PeekRequest; +import com.phonepe.magazine.response.FireHistoryResponse; +import com.phonepe.magazine.response.FireHistorySpan; +import com.phonepe.magazine.response.FireHistoryWindow; import com.phonepe.magazine.response.MagazineDescriptor; import com.phonepe.magazine.response.MagazineMetadataResponse; import com.phonepe.magazine.response.PeekResponse; @@ -32,6 +38,8 @@ import jakarta.ws.rs.BadRequestException; import jakarta.ws.rs.NotFoundException; import java.time.Duration; +import java.time.Instant; +import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.LinkedHashMap; @@ -39,6 +47,8 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.SortedMap; +import java.util.TreeMap; public final class MagazineService { @@ -95,6 +105,93 @@ private MagazineMetadataResponse readMetadata(final String identifier) { totalMetadata(shards.values())); } + /** + * Retained delivery history, reduced to one row per checkpoint window, newest first. + *

+ * The whole retained record rather than an answer about one moment: it is bounded by + * configuration, needs no input from the operator, and shows both how delivery progressed and + * how far back the magazine can be asked about at all. + * + * @param shard a single shard to report on, or null for totals across every shard. Shard count + * is unbounded, so the per-shard series is fetched one shard at a time rather than + * shipped as a grid. + */ + public FireHistoryResponse fireHistory(final String identifier, final String shard) { + final Magazine magazine = magazine(identifier); + final Map> history; + try { + history = magazine.fireHistory(); + } catch (MagazineException e) { + if (e.getErrorCode() == ErrorCode.NOT_ENABLED) { + return new FireHistoryResponse(identifier, false, List.of(), shard, null, List.of()); + } + throw e; + } + + final List shards = history.keySet().stream() + .sorted(MagazineService::compareShardIds) + .toList(); + if (Objects.nonNull(shard) && !history.containsKey(shard)) { + throw new BadRequestException("No recorded history for shard: " + shard); + } + final Map> selected = Objects.isNull(shard) + ? history + : Map.of(shard, history.get(shard)); + + // Newest first, so the window keys sort descending. + final SortedMap> pointers = new TreeMap<>(Comparator.reverseOrder()); + selected.forEach((shardId, checkpoints) -> checkpoints.forEach(checkpoint -> pointers + .computeIfAbsent(checkpoint.recordedAt(), key -> new LinkedHashMap<>()) + .put(shardId, checkpoint.firePointer()))); + + // Deltas need the older neighbour, so walk oldest first while filling newest first. + final Instant now = Instant.now(); + final List>> ordered = new ArrayList<>(pointers.entrySet()); + final FireHistoryWindow[] windows = new FireHistoryWindow[ordered.size()]; + Map older = null; + for (int index = ordered.size() - 1; index >= 0; index--) { + final Map.Entry> entry = ordered.get(index); + windows[index] = toWindow(entry.getKey(), entry.getValue(), older, now); + older = entry.getValue(); + } + return new FireHistoryResponse(identifier, true, shards, shard, + span(pointers, shards, magazine.getShards(), now), List.of(windows)); + } + + private static FireHistoryWindow toWindow(final Instant recordedAt, + final Map pointers, + final Map older, + final Instant now) { + long total = 0; + long delivered = 0; + boolean comparable = false; + for (Map.Entry entry : pointers.entrySet()) { + total += entry.getValue(); + // Only shards present in both windows contribute. One that appeared in between would + // otherwise report its entire pointer as having been delivered in this window. + final Long previous = Objects.isNull(older) ? null : older.get(entry.getKey()); + if (Objects.nonNull(previous)) { + delivered += entry.getValue() - previous; + comparable = true; + } + } + return new FireHistoryWindow(recordedAt, Duration.between(recordedAt, now).getSeconds(), + pointers.size(), total, comparable ? delivered : null); + } + + private static FireHistorySpan span(final SortedMap> pointers, + final List shards, + final int shardsConfigured, + final Instant now) { + if (pointers.isEmpty()) { + return null; + } + final Instant newest = pointers.firstKey(); + final Instant oldest = pointers.lastKey(); + return new FireHistorySpan(oldest, newest, Duration.between(oldest, now).getSeconds(), + pointers.size(), shards.size(), shardsConfigured); + } + public PeekResponse peek(final String identifier, final PeekRequest request) { if (Objects.isNull(request) || Objects.isNull(request.pointers()) || request.pointers().isEmpty()) { throw new BadRequestException("pointers must contain at least one shard"); diff --git a/magazine-dw-bundle/src/main/resources/magazineAssets/magazineIndex.html b/magazine-dw-bundle/src/main/resources/magazineAssets/magazineIndex.html index bec9b64..2b57f4e 100644 --- a/magazine-dw-bundle/src/main/resources/magazineAssets/magazineIndex.html +++ b/magazine-dw-bundle/src/main/resources/magazineAssets/magazineIndex.html @@ -12,9 +12,10 @@

Waiting for metadata
- +
Select a magazine to inspect metadata.
+
Select a magazine to inspect its delivery history.

Non-consuming peek

@@ -31,12 +32,14 @@

Non-consuming peek

const api = `${dashboardPath}/magazine/v1/magazines`; let selected = null; let selectionVersion = 0; + let historyShard = ''; const esc = value => String(value).replace(/[&<>'"]/g, char => ({'&':'&','<':'<','>':'>',"'":''','"':'"'}[char])); async function json(url, options) { const response=await fetch(url,options); if(!response.ok) throw new Error(`${response.status} ${await response.text()}`); return response.json(); } const number = value => Number(value).toLocaleString(); async function choose(identifier, button) { const version=++selectionVersion; selected=identifier; document.querySelectorAll('nav button').forEach(item=>item.classList.remove('active')); button.classList.add('active'); document.querySelector('#peek').disabled=false; document.querySelector('#result').className='peek-result empty'; document.querySelector('#result').textContent='Enter a shard and pointer to inspect one record.'; - await refreshMetadata(identifier, version); + historyShard=''; + await Promise.all([refreshMetadata(identifier, version), refreshHistory(identifier, version)]); } async function refreshMetadata(identifier, version) { try { const value=await json(`${api}/${encodeURIComponent(identifier)}/metadata`); if(version===selectionVersion){renderMetadata(value);document.querySelector('#last-updated').textContent=`Updated ${new Date().toLocaleTimeString()} · refreshes every 30s`;} } catch(error) { if(version===selectionVersion)document.querySelector('#metadata').innerHTML=`
${esc(error.message)}
`; } @@ -46,6 +49,27 @@

Non-consuming peek

const rows=Object.entries(value.shards).map(([shard,data])=>`${esc(shard)}${number(data.loadCounter)}${number(data.fireCounter)}${number(data.loadPointer)}${number(data.firePointer)}${number(data.pending)}`).join(''); document.querySelector('#metadata').innerHTML=`

${esc(value.identifier)}

${[['load counter','loadCounter'],['fire counter','fireCounter'],['pending','pending']].map(metric).join('')}
${rows}
ShardLoad counterFire counterLoad pointerFire pointerPending
`; } + async function refreshHistory(identifier, version) { + const query=historyShard?`?shard=${encodeURIComponent(historyShard)}`:''; + try { const value=await json(`${api}/${encodeURIComponent(identifier)}/fire-history${query}`); if(version===selectionVersion)renderHistory(value); } catch(error) { if(version===selectionVersion)document.querySelector('#history').innerHTML=`

Delivery history

${esc(error.message)}
`; } + } + const duration=seconds=>seconds<60?`${number(seconds)}s`:seconds<3600?`${number(Math.floor(seconds/60))}m ${number(seconds%60)}s`:`${number(Math.floor(seconds/3600))}h ${number(Math.floor(seconds%3600/60))}m`; + function renderHistory(value) { + const panel=document.querySelector('#history'); + if(!value.enabled){panel.innerHTML=`

Delivery history

Fire history is not enabled for this magazine. Set fireHistoryEnabled on the storage configuration to record where each shard's fire pointer stood over time.
`;return;} + if(!value.windows.length){panel.innerHTML='

Delivery history

No checkpoints recorded yet. Normal for a magazine that has not been consumed from.
';return;} + const span=value.span; + const scope=value.shard?esc(value.shard):`all ${number(span.shardsReporting)} recording shards`; + const options=[``].concat(value.shards.map(shard=>``)).join(''); + const summary=[ + [`${number(span.windows)}`,'windows retained'], + [duration(span.spanSeconds),'reaches back'], + [`${number(span.shardsReporting)} / ${number(span.shardsConfigured)}`,'shards recording'] + ].map(([value,label])=>`
${esc(value)}${label}
`).join(''); + const rows=value.windows.map(window=>`${esc(new Date(window.recordedAt).toLocaleString())}${esc(duration(window.ageSeconds))}${value.shard?'':`${number(window.shardsReporting)}`}${number(window.firePointer)}${window.delivered===null?'-':number(window.delivered)}`).join(''); + panel.innerHTML=`

Delivery history

${summary}

Fire pointer at each recorded window for ${scope}, newest first. Every slot at or below a pointer had been handed to a consumer by that time. A redrive asking about anything older than the oldest row gets an error rather than an answer.

${value.shard?'':''}${rows}
Recorded atAgeShardsFire pointerDelivered
`; + document.querySelector('#history-shard').addEventListener('change',event=>{historyShard=event.target.value;refreshHistory(selected,selectionVersion);}); + } function renderPeek(value) { const result=document.querySelector('#result'); if (!value.data.length) { result.className='peek-result empty'; result.textContent='No record found at this shard and pointer.'; return; } @@ -54,8 +78,9 @@

Non-consuming peek

} document.querySelector('#peek-form').addEventListener('submit',async event=>{ event.preventDefault(); const result=document.querySelector('#result'); const shard=document.querySelector('#peek-shard').value; const pointer=document.querySelector('#peek-pointer').value; if(!/^\d+$/.test(shard)||!/^\d+$/.test(pointer)||Number(pointer)<1){result.className='peek-result error';result.textContent='Enter a non-negative shard and positive pointer.';return;} if(!Number.isSafeInteger(Number(pointer))){result.className='peek-result error';result.textContent='Pointer is too large to represent exactly in the browser.';return;} try { renderPeek(await json(`${api}/${encodeURIComponent(selected)}/peek`,{method:'POST',headers:{'Content-Type':'application/json'},body:JSON.stringify({pointers:{[shard]:[Number(pointer)]}})})); } catch(error) { result.className='peek-result error'; result.textContent=error.message; } }); - json(api).then(items=>{ const nav=document.querySelector('#magazines'); if(!items.length){nav.innerHTML='
No magazines configured.
';return;} nav.innerHTML=''; items.forEach((item,index)=>{const button=document.createElement('button');button.textContent=item.identifier;button.addEventListener('click',()=>choose(item.identifier,button));nav.appendChild(button);if(index===0)choose(item.identifier,button);}); }).catch(error=>document.querySelector('#magazines').innerHTML=`
${esc(error.message)}
`); - setInterval(()=>{if(selected)refreshMetadata(selected,selectionVersion);},30000); + json(api).then(items=>{ const nav=document.querySelector('#magazines'); if(!items.length){nav.innerHTML='
No magazines configured.
';return;} nav.innerHTML=''; items.forEach((item,index)=>{const button=document.createElement('button');button.textContent=item.identifier;button.addEventListener('click',()=>choose(item.identifier,button));nav.appendChild(button);if(index===0)choose(item.identifier,button);}); const none=document.createElement('div');none.className='empty no-match';none.textContent='No magazine matches that filter.';none.hidden=true;nav.appendChild(none); }).catch(error=>document.querySelector('#magazines').innerHTML=`
${esc(error.message)}
`); + document.querySelector('#magazine-filter').addEventListener('input',event=>{ const term=event.target.value.trim().toLowerCase(); document.querySelectorAll('#magazines button').forEach(button=>{button.hidden=term.length>0&&!button.textContent.toLowerCase().includes(term);}); const none=document.querySelector('#magazines .no-match'); if(none)none.hidden=[...document.querySelectorAll('#magazines button')].some(button=>!button.hidden); }); + setInterval(()=>{if(selected){refreshMetadata(selected,selectionVersion);refreshHistory(selected,selectionVersion);}},30000); diff --git a/magazine-dw-bundle/src/main/resources/magazineAssets/style.css b/magazine-dw-bundle/src/main/resources/magazineAssets/style.css index 572a662..edf8e48 100644 --- a/magazine-dw-bundle/src/main/resources/magazineAssets/style.css +++ b/magazine-dw-bundle/src/main/resources/magazineAssets/style.css @@ -22,6 +22,14 @@ table { width:100%; border-collapse:collapse; font-variant-numeric:tabular-nums; th,td { text-align:right; border-bottom:1px solid var(--line); padding:10px 8px; } th:first-child,td:first-child { text-align:left; } th { color:var(--muted); font-size:11px; text-transform:uppercase; } +.filter { width:100%; margin-bottom:14px; color:var(--ink); background:#0d120e; border:1px solid var(--line); padding:10px 12px; } +.filter:focus { outline:1px solid var(--accent); border-color:var(--accent); } +.note { color:var(--muted); font-size:12px; margin:0 0 16px; } +.history-metrics { margin-bottom:16px; } +.scope { display:flex; align-items:center; gap:10px; margin-bottom:12px; } +.scope label { color:var(--muted); font-size:11px; letter-spacing:.08em; text-transform:uppercase; } +.scope select { color:var(--ink); background:#0d120e; border:1px solid var(--line); padding:9px 12px; } +.scope select:focus { outline:1px solid var(--accent); border-color:var(--accent); } .peek { margin-top:20px; display:grid; grid-template-columns:140px minmax(180px,1fr) auto; gap:12px; align-items:end; } .peek label { color:var(--muted); font-size:11px; letter-spacing:.08em; text-transform:uppercase; } .peek input { display:block; width:100%; margin-top:6px; color:var(--ink); background:#0d120e; border:1px solid var(--line); padding:11px 12px; } diff --git a/magazine-dw-bundle/src/test/java/com/phonepe/magazine/MagazineDashboardAssetsTest.java b/magazine-dw-bundle/src/test/java/com/phonepe/magazine/MagazineDashboardAssetsTest.java index c0a6a7d..229a562 100644 --- a/magazine-dw-bundle/src/test/java/com/phonepe/magazine/MagazineDashboardAssetsTest.java +++ b/magazine-dw-bundle/src/test/java/com/phonepe/magazine/MagazineDashboardAssetsTest.java @@ -38,12 +38,21 @@ void dashboardAssetsAreResponsiveAndReadOnly() throws IOException { assertTrue(html.contains("href=\"style.css\"")); assertTrue(html.contains("load counter")); assertTrue(html.contains("fire counter")); + assertTrue(html.contains("id=\"magazine-filter\"")); + assertTrue(html.contains("/fire-history")); + assertTrue(html.contains("Delivery history")); + assertTrue(html.contains("id=\"history-shard\"")); + assertTrue(html.contains("windows retained")); + assertFalse(html.contains("window.firePointers"), + "history is reduced server-side; a per-shard grid does not survive 256 shards"); + assertTrue(css.contains(".filter")); assertTrue(html.contains("setInterval")); assertTrue(html.contains("30000")); assertTrue(css.contains("@media (max-width:760px)")); assertFalse(html.contains("/load")); assertFalse(html.contains("/reload")); - assertFalse(html.contains("/fire")); + assertFalse(html.replace("/fire-history", "").contains("/fire"), + "/fire-history is a read; the console must never call the mutating /fire"); assertFalse(html.contains("method:'DELETE'")); } diff --git a/magazine-dw-bundle/src/test/java/com/phonepe/magazine/demo/MagazineDashboardDemoApplication.java b/magazine-dw-bundle/src/test/java/com/phonepe/magazine/demo/MagazineDashboardDemoApplication.java index f3a2672..eab07df 100644 --- a/magazine-dw-bundle/src/test/java/com/phonepe/magazine/demo/MagazineDashboardDemoApplication.java +++ b/magazine-dw-bundle/src/test/java/com/phonepe/magazine/demo/MagazineDashboardDemoApplication.java @@ -19,12 +19,17 @@ import com.phonepe.magazine.Magazine; import com.phonepe.magazine.MagazineBundle; import com.phonepe.magazine.config.MagazineBundleConfiguration; +import com.phonepe.magazine.entity.FireCheckpoint; import com.phonepe.magazine.entity.MagazineData; import com.phonepe.magazine.entity.MetaData; +import com.phonepe.magazine.exception.ErrorCode; +import com.phonepe.magazine.exception.MagazineExceptions; import io.dropwizard.core.Application; import io.dropwizard.core.setup.Bootstrap; import io.dropwizard.core.setup.Environment; +import java.time.Duration; +import java.time.Instant; import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; @@ -81,7 +86,12 @@ public void run(final MagazineDashboardDemoConfiguration configuration, final En Set.of( new MagazineData<>("welcome-email:user-1042", 35, 0, "email-jobs"), new MagazineData<>("receipt-email:order-8821", 36, 0, "email-jobs"), - new MagazineData<>("password-reset:user-991", 31, 1, "email-jobs"))); + new MagazineData<>("password-reset:user-991", 31, 1, "email-jobs")), + // SHARD_1 only starts recording partway through, as a shard does when it is first + // loaded to, so the oldest windows show one shard reporting rather than two. + Map.of( + "SHARD_0", checkpoints(34, 6, 8), + "SHARD_1", checkpoints(29, 5, 5))); final Magazine paymentRetries = magazine( "payment-retries", Map.of( @@ -90,18 +100,41 @@ public void run(final MagazineDashboardDemoConfiguration configuration, final En Set.of( new MagazineData<>("payment:PAY-202601", 50, 0, "payment-retries"), new MagazineData<>("payment:PAY-202602", 51, 0, "payment-retries"), - new MagazineData<>("payment:PAY-202603", 47, 1, "payment-retries"))); + new MagazineData<>("payment:PAY-202603", 47, 1, "payment-retries")), + // Fire history left off, so the dashboard shows the disabled state rather than an error. + null); magazineBundle.getMagazineManager().refresh(java.util.List.of(emailJobs, paymentRetries)); } + /** + * A descending run of checkpoints on five-minute windows, newest first, as the recorder would + * have written them. + */ + private static java.util.List checkpoints(final long newest, final long perWindow, + final int windows) { + final java.util.List checkpoints = new java.util.ArrayList<>(windows); + for (int window = 0; window < windows; window++) { + checkpoints.add(new FireCheckpoint(newest - (long) window * perWindow, + Instant.now().minus(Duration.ofMinutes(5L * window + 2)))); + } + return checkpoints; + } + @SuppressWarnings("unchecked") private static Magazine magazine(final String identifier, final Map metadata, - final Set> records) { + final Set> records, + final Map> checkpoints) { final Magazine magazine = mock(Magazine.class); when(magazine.getMagazineIdentifier()).thenReturn(identifier); when(magazine.getShards()).thenReturn(metadata.size()); when(magazine.getMetaData()).thenReturn(new LinkedHashMap<>(metadata)); + if (checkpoints == null) { + when(magazine.fireHistory()).thenThrow( + MagazineExceptions.of(ErrorCode.NOT_ENABLED, "Fire history is not enabled for " + identifier)); + } else { + when(magazine.fireHistory()).thenReturn(new LinkedHashMap<>(checkpoints)); + } when(magazine.peek(anyMap())).thenAnswer(invocation -> { final Map> requested = invocation.getArgument(0); return records.stream() diff --git a/magazine-dw-bundle/src/test/java/com/phonepe/magazine/resources/MagazineResourceTest.java b/magazine-dw-bundle/src/test/java/com/phonepe/magazine/resources/MagazineResourceTest.java index e4411a5..b2ca45a 100644 --- a/magazine-dw-bundle/src/test/java/com/phonepe/magazine/resources/MagazineResourceTest.java +++ b/magazine-dw-bundle/src/test/java/com/phonepe/magazine/resources/MagazineResourceTest.java @@ -19,8 +19,11 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.phonepe.magazine.Magazine; import com.phonepe.magazine.MagazineManager; +import com.phonepe.magazine.entity.FireCheckpoint; import com.phonepe.magazine.entity.MagazineData; import com.phonepe.magazine.entity.MetaData; +import com.phonepe.magazine.exception.ErrorCode; +import com.phonepe.magazine.exception.MagazineExceptions; import com.phonepe.magazine.request.PeekRequest; import com.phonepe.magazine.response.*; import com.phonepe.magazine.service.MagazineService; @@ -38,6 +41,7 @@ import org.mockito.Mockito; import java.lang.reflect.Method; +import java.time.Instant; import java.util.Collections; import java.util.List; import java.util.Map; @@ -52,6 +56,10 @@ @ExtendWith(DropwizardExtensionsSupport.class) class MagazineResourceTest { + private static final Instant OLDEST = Instant.parse("2026-01-01T10:00:00Z"); + private static final Instant MIDDLE = Instant.parse("2026-01-01T10:05:00Z"); + private static final Instant NEWEST = Instant.parse("2026-01-01T10:10:00Z"); + private final Magazine magazine = mockMagazine(); private final MagazineManager magazineManager = mock(MagazineManager.class); private final MagazineService service = new MagazineService(magazineManager, new ObjectMapper()); @@ -80,6 +88,144 @@ void listsOnlyConfiguredMagazines() { verifyNoDataOperations(); } + private void stubHistory() { + // SHARD_0 missed the oldest window - it may not have existed yet. + when(magazine.fireHistory()).thenReturn(Map.of( + "SHARD_1", List.of(new FireCheckpoint(512, NEWEST), new FireCheckpoint(500, MIDDLE), + new FireCheckpoint(400, OLDEST)), + "SHARD_0", List.of(new FireCheckpoint(340, NEWEST), new FireCheckpoint(300, MIDDLE)))); + } + + private FireHistoryResponse history(final String shard) { + var target = resources.target("/magazine/v1/magazines/jobs/fire-history"); + if (shard != null) { + target = target.queryParam("shard", shard); + } + return target.request().get(FireHistoryResponse.class); + } + + @Test + void reportsRetainedHistoryAsOneRowPerWindowNewestFirst() { + stubHistory(); + + final FireHistoryResponse response = history(null); + + assertTrue(response.enabled()); + assertNull(response.shard(), "totals by default"); + assertEquals(List.of("SHARD_0", "SHARD_1"), response.shards()); + assertEquals(List.of(NEWEST, MIDDLE, OLDEST), + response.windows().stream().map(FireHistoryWindow::recordedAt).toList()); + assertEquals(852, response.windows().get(0).firePointer(), "totalled across reporting shards"); + assertEquals(2, response.windows().get(0).shardsReporting()); + assertEquals(1, response.windows().get(2).shardsReporting()); + verifyNoDataOperations(); + } + + /** + * The row count must not grow with shard count - a magazine may have hundreds of shards, and + * the payload is polled every 30 seconds. + */ + @Test + void rowCountIsIndependentOfShardCount() { + final Instant window = Instant.parse("2026-01-01T10:00:00Z"); + final Map> wide = new java.util.LinkedHashMap<>(); + for (int shard = 0; shard < 256; shard++) { + wide.put("SHARD_" + shard, List.of(new FireCheckpoint(10, window))); + } + when(magazine.fireHistory()).thenReturn(wide); + when(magazine.getShards()).thenReturn(256); + + final FireHistoryResponse response = history(null); + + assertEquals(1, response.windows().size(), "one row per window, whatever the shard count"); + assertEquals(2560, response.windows().get(0).firePointer()); + assertEquals(256, response.span().shardsReporting()); + assertEquals(256, response.span().shardsConfigured()); + } + + /** + * A shard that only appears in the newer window must not report its whole pointer as having + * been delivered in that window. + */ + @Test + void deliveredCountsOnlyShardsPresentInBothWindows() { + stubHistory(); + + final List windows = history(null).windows(); + + assertNull(windows.get(2).delivered(), "the oldest row has nothing to compare against"); + assertEquals(100, windows.get(1).delivered(), "only SHARD_1 spans both, 500 - 400"); + assertEquals(52, windows.get(0).delivered(), "both shards span, (512-500) + (340-300)"); + } + + @Test + void reportsHowFarBackHistoryReaches() { + stubHistory(); + + final FireHistorySpan span = history(null).span(); + + assertEquals(OLDEST, span.oldest()); + assertEquals(NEWEST, span.newest()); + assertEquals(3, span.windows()); + assertEquals(2, span.shardsReporting()); + assertEquals(4, span.shardsConfigured()); + } + + @Test + void drillsIntoASingleShard() { + stubHistory(); + + final FireHistoryResponse response = history("SHARD_0"); + + assertEquals("SHARD_0", response.shard()); + assertEquals(List.of(NEWEST, MIDDLE), + response.windows().stream().map(FireHistoryWindow::recordedAt).toList(), + "only the windows that shard recorded"); + assertEquals(340, response.windows().get(0).firePointer(), "that shard's pointer, not a total"); + assertEquals(List.of("SHARD_0", "SHARD_1"), response.shards(), "the selector still lists every shard"); + } + + @Test + void rejectsAShardWithNoRecordedHistory() { + stubHistory(); + + try (Response response = resources.target("/magazine/v1/magazines/jobs/fire-history") + .queryParam("shard", "SHARD_3") + .request() + .get()) { + assertEquals(400, response.getStatus()); + } + } + + /** + * A magazine that does not record checkpoints is a configuration fact, not a failure. Surfacing + * it as a 500 would make an unconfigured dashboard look like a broken one. + */ + @Test + void reportsFireHistoryAsDisabledRatherThanFailing() { + when(magazine.fireHistory()).thenThrow(MagazineExceptions.of(ErrorCode.NOT_ENABLED, "not enabled")); + + final FireHistoryResponse response = history(null); + + assertFalse(response.enabled()); + assertTrue(response.windows().isEmpty()); + assertNull(response.span()); + } + + /** + * Browsing history must never write to the magazine being inspected. firePointerBefore records + * a checkpoint as a side effect; fireHistory deliberately does not, and the console uses that. + */ + @Test + void browsingHistoryDoesNotRecordAnything() { + when(magazine.fireHistory()).thenReturn(Map.of()); + + assertNull(history(null).span()); + + verify(magazine, never()).firePointerBefore(any()); + verifyNoDataOperations(); + } + @Test void reflectsMagazineManagerRefreshes() { final Magazine replacement = mockMagazine(); @@ -274,7 +420,10 @@ void webApiDoesNotExposeStorageOrMutationMethods() { ShardMetadata.class, PeekRequest.class, PeekResponse.class, - PeekedData.class)) { + PeekedData.class, + FireHistoryResponse.class, + FireHistoryWindow.class, + FireHistorySpan.class)) { for (Method method : type.getDeclaredMethods()) { assertFalse(methods.contains(method.getName()), type + " exposes " + method.getName()); assertFalse(method.getReturnType().getName().contains("BaseMagazineStorage")); From 75b66c0d8739b49222be45c347541888537e282d Mon Sep 17 00:00:00 2001 From: Shantanu Tiwari Date: Wed, 16 Sep 2026 17:14:58 +0530 Subject: [PATCH 03/10] Add fire history docs --- docs/docs/backends/aerospike.md | 23 +++++- docs/docs/concepts/api-reference.md | 2 + docs/docs/concepts/defaults.md | 5 ++ docs/docs/usage.md | 71 +++++++++++++++++++ .../aerospike/MagazineFireHistoryTest.java | 23 ++++-- 5 files changed, 119 insertions(+), 5 deletions(-) diff --git a/docs/docs/backends/aerospike.md b/docs/docs/backends/aerospike.md index 77f2151..0e5fdb6 100644 --- a/docs/docs/backends/aerospike.md +++ b/docs/docs/backends/aerospike.md @@ -46,7 +46,7 @@ Config fields and their defaults are documented in [Defaults](../concepts/defaul | Record | Key | Bins | |---|---|---| | Shard configuration | `_SHARDS` | `SHARDS`, `META_VERSION`, `CREATED_AT` | -| Metadata (unified) | `_SHARD__METADATA` | `LOAD_POINTER`, `FIRE_POINTER`, `LOAD_COUNTER`, `FIRE_COUNTER` | +| Metadata (unified) | `_SHARD__METADATA` | `LOAD_POINTER`, `FIRE_POINTER`, `LOAD_COUNTER`, `FIRE_COUNTER`, and `FIRE_HISTORY` when enabled | | Metadata (legacy) | `_SHARD__POINTERS` and `..._COUNTERS` | pointers / counters split across two records | | Data | `_SHARD__` | `data` | | Dedupe marker | `` in `_deduper` | `modified_at` | @@ -122,6 +122,27 @@ budget on shards containing nothing but failed writes. The ledger is safe because `FIRE_COUNTER` can never over-count — it advances only after a payload was actually read, so a crash leaves it short, which keeps the shard active and fails open. +## Fire history + +When `fireHistoryEnabled` is set, each shard's **unified metadata record** carries an extra map bin +holding delivery-time checkpoints: window start to the fire pointer reached in that window. + +- **One entry per shard per window**, regardless of throughput. +- **Checkpoints are written during the active-shard refresh, not on the claim path.** That refresh + already batch-reads every shard's pointer record, so the numbers being recorded are already in + hand and no extra read is needed. Deliberately *not* coupled to the claim: claims are the hot path + and are far more frequent than the resolution a window needs. +- `firePointerBefore` also records before it resolves, so a magazine whose history cannot yet answer + still accumulates usable history rather than being stuck unable to ever answer. +- **Each shard's checkpoint is claimed independently**, and the claim is released if the write fails, + so a shard that errored is retried next round instead of sitting the window out. +- **Eviction is by count** (`fireHistoryEntries`), applied on write — by count rather than age so an + idle magazine keeps reach instead of having its history wiped by the first write after a pause. +- **Legacy split metadata does not support it.** Fire history requires the unified metadata record. + +The cost is bytes on a record that is already read and written by a periodic refresh, not an extra +operation on the delivery path. + ## Metadata schema versions `META_VERSION=2` keeps pointers and counters in one record; `0` splits them. Both are supported and diff --git a/docs/docs/concepts/api-reference.md b/docs/docs/concepts/api-reference.md index 2135320..665013a 100644 --- a/docs/docs/concepts/api-reference.md +++ b/docs/docs/concepts/api-reference.md @@ -36,6 +36,8 @@ Magazine magazine = Magazine.builder() | `getShards()` | `int` | This magazine's **persisted** shard count (from the resolved `MagazineContext`), not the shard count configured on the storage. | | `peek(Map> shardPointersMap)` | `Set>` | Read specific records without consuming. | | `getMagazineIdentifier()` | `String` | The identifier this magazine is bound to. | +| `firePointerBefore(Instant instant)` | `Map` | Where each shard's fire pointer stood at `instant`. Requires `fireHistoryEnabled`. A shard is **absent** when no checkpoint reaches back that far — treat that as "do nothing here", not "empty". Throws `NOT_ENABLED`, or `INVALID_REQUEST` when the retained history no longer spans `instant`. | +| `fireHistory()` | `Map>` | Every retained checkpoint per shard, newest first. Requires `fireHistoryEnabled`. | !!! warning "`fire()` is at-most-once" A record is claimed by a guarded atomic increment of the fire pointer. If the client times out after the server applied the claim, the pointer has advanced and that record will never be delivered. `fire()` throws `NOTHING_TO_FIRE` when the magazine is drained and `RETRIES_EXHAUSTED` when it gave up while skipping pointer holes — the latter does **not** mean the queue is empty. See [Delivery Semantics](delivery-semantics.md). diff --git a/docs/docs/concepts/defaults.md b/docs/docs/concepts/defaults.md index 5d79ac0..a0a7c8e 100644 --- a/docs/docs/concepts/defaults.md +++ b/docs/docs/concepts/defaults.md @@ -14,6 +14,9 @@ | `activeShardRefreshSeconds` | `5` | Primary lever on steady-state read load. | | `maxFireHoleSkips` | `512` | Bounds `fire()`; exhausting it raises `RETRIES_EXHAUSTED`. | | `metricsEnabled` | `true` | Publishes to Micrometer's global registry unless a `meterRegistry` is set on the builder. | +| `fireHistoryEnabled` | `false` | Records delivery-time checkpoints, which `firePointerBefore` reads. Off by default: the checkpoint map rides the pointer record. | +| `fireHistoryWindowSeconds` | `300` | Seconds per checkpoint, and so the resolution of every `firePointerBefore` answer. | +| `fireHistoryEntries` | `32` | Checkpoints retained per shard. Evicted by **count**, not age. | ### `metaDataTtl` must outlive `recordTtl` @@ -65,6 +68,8 @@ Not user-configurable. Defined in `com.phonepe.magazine.impl.aerospike.common.Ae | `MAX_FIRE_HOLE_SKIPS` | `512` | Default for `maxFireHoleSkips`. | | `DEFAULT_SHARDS` | `8` | Default for `shards`. | | `DEFAULT_REFRESH` | `5` | Default for `activeShardRefreshSeconds`. | +| `DEFAULT_FIRE_HISTORY_WINDOW_SECONDS` | `300` | Default for `fireHistoryWindowSeconds`. | +| `DEFAULT_FIRE_HISTORY_ENTRIES` | `32` | Default for `fireHistoryEntries`. | | `DEFAULT_MAX_ELEMENTS` | `1024` | Active-shard cache capacity, in magazines. | | `SHARD_CONFIGURATION_TTL_SECONDS` | `157680000` (5 years) | TTL of the per-magazine shard configuration record. | | `LEGACY_METADATA_SCHEMA_VERSION` | `0` | Pointers and counters in separate records. | diff --git a/docs/docs/usage.md b/docs/docs/usage.md index 795349e..7a61efc 100644 --- a/docs/docs/usage.md +++ b/docs/docs/usage.md @@ -143,6 +143,77 @@ SHARD_2 load=0/0 fire=0/0 ... ``` +## Delivery-Time History + +Answers "where had each shard's fire pointer reached at time T?" — which is how a caller can tell an +abandoned in-flight record from one that was only just claimed, without stamping a timestamp on every +message. + +**Off by default.** The checkpoint map rides the shard's pointer record and every claim rewrites that +record, so a caller that never asks should not pay for it. + +```java +AerospikeStorageConfig.builder() + .dataSetName("my_data") + .metaSetName("my_meta") + .namespace("test") + .shards(8) + .fireHistoryEnabled(true) // default false + .fireHistoryWindowSeconds(60) // resolution of every answer + .fireHistoryEntries(32) // checkpoints retained per shard + .build(); +``` + +### Asking + +```java +Map watermarks = + magazine.firePointerBefore(Instant.now().minus(Duration.ofMinutes(20))); + +watermarks.forEach((shard, checkpoint) -> + System.out.printf("%s: everything at or below pointer %d was claimed 20+ minutes ago%n", + shard, checkpoint.firePointer())); +``` + +Every slot at or below the returned pointer was claimed at or before that instant. Slots above it +might have been claimed a moment ago. + +!!! note "An absent shard means 'do nothing', not 'empty'" + A shard is missing from the result when no checkpoint reaches back that far yet. That is normal + for a young magazine. Treat it as "nothing is provably old enough here this round" — never as + "this shard has nothing in flight". + +### Sizing, and the failure you must not swallow + +| Setting | Meaning | +|---|---| +| `fireHistoryWindowSeconds` | Seconds covered by one checkpoint, and therefore the **resolution** of every answer. One entry per shard per window, whatever the throughput | +| `fireHistoryEntries` | Checkpoints retained per shard. Eviction is by **count, not age**, so an intermittently used magazine reaches further back than `window x entries` rather than having its history wiped by the first write after an idle period | + +Asking about an instant further back than the retained history throws `INVALID_REQUEST`, and asking +at all with the feature disabled throws `NOT_ENABLED`. + +**This is deliberate and must not be caught and ignored.** Returning an empty map would be +indistinguishable from "nothing is old enough", and a caller using this to recover abandoned messages +would silently stop recovering them. Size `window x entries` to comfortably exceed the oldest age you +intend to ask about. + +```java +try { + magazine.firePointerBefore(cutoff); +} catch (MagazineException e) { + if (e.getErrorCode() == ErrorCode.INVALID_REQUEST) { + // History no longer spans the cutoff. Skip this round loudly - do not treat as "nothing to do". + } +} +``` + +### Inspecting the whole history + +```java +Map> history = magazine.fireHistory(); // newest first per shard +``` + ## Managing Multiple Magazines `MagazineManager` acts as a registry for multiple `Magazine` instances, potentially of different types. diff --git a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java index d26e7f6..9096d3b 100644 --- a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java +++ b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java @@ -190,6 +190,7 @@ void readingRecordsACheckpoint() throws Exception { final Magazine magazine = magazine("HIST_BURST", "HIST_BURST_SET", true, 1, 8); loadAndFire(magazine, 3); magazine.firePointerBefore(Instant.now()); + final long newestBefore = newestWindowKey(magazine); Thread.sleep(1_100); // No load, no fire: the magazine is idle. Reading alone must still move history forward, @@ -197,8 +198,13 @@ void readingRecordsACheckpoint() throws Exception { // checkpoint predating the burst and its orphans would never become visible. magazine.firePointerBefore(Instant.now()); - assertEquals(2, history(magazine, 0).size(), - "the read should have recorded the new window by itself"); + // The property, not a count. The active-shard refresh records on its own schedule, so how + // many windows exist by this point is not something the test controls - under load the + // preceding loadAndFire can contribute one of its own. What the read must guarantee is that + // a window strictly newer than anything present beforehand now exists. + assertTrue(newestWindowKey(magazine) < newestBefore, + "the read should have recorded a newer window by itself; keys are negated window " + + "timestamps, so newest sorts lowest"); } @Test @@ -392,8 +398,17 @@ private int shardsWithMetadata(final Magazine magazine, final int shards return found; } - private int totalCheckpoints(final Magazine magazine, final int shards) { - int total = 0; + /** + * Keys are negated window timestamps on a KEY_ORDERED map, so the newest window is the lowest + * key. Returns 0 when nothing has been recorded, which no real window can equal. + */ + private long newestWindowKey(final Magazine magazine) { + final Map history = history(magazine, 0); + assertNotNull(history, "expected a checkpoint map on the pointer record"); + return history.keySet().stream().mapToLong(Long::longValue).min().orElse(0L); + } + + private int totalCheckpoints(final Magazine magazine, final int shards) { int total = 0; for (int shard = 0; shard < shards; shard++) { final Map history = history(magazine, shard); total += history == null ? 0 : history.size(); From 1d7b9d35aa84e8731669bfb4ecf6b3efd9e9eafc Mon Sep 17 00:00:00 2001 From: Shantanu Tiwari Date: Fri, 18 Sep 2026 16:03:44 +0530 Subject: [PATCH 04/10] Add batch delete support --- README.md | 1 + docs/docs/concepts/api-reference.md | 2 + docs/docs/concepts/metrics.md | 5 +- docs/docs/index.md | 2 +- docs/docs/upgrading.md | 3 + docs/docs/usage.md | 5 ++ .../java/com/phonepe/magazine/Magazine.java | 14 +++ .../magazine/core/BaseMagazineStorage.java | 9 ++ .../impl/aerospike/AerospikeStorage.java | 32 +++++-- .../aerospike/common/AerospikePolicies.java | 14 +++ .../impl/aerospike/common/ErrorMessage.java | 2 + .../aerospike/store/MagazineDataStore.java | 36 ++++++++ .../magazine/metrics/StorageOperation.java | 1 + .../impl/aerospike/MagazineLifecycleTest.java | 89 +++++++++++++++++++ .../impl/aerospike/MagazineMetricsTest.java | 50 +++++++++++ 15 files changed, 258 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 096592a..2f072ae 100644 --- a/README.md +++ b/README.md @@ -174,6 +174,7 @@ For peek, try shard `0` pointers `35` and `36` on `email-jobs`, or pointers `50` | | `fire()` | Dequeue next item | | | `reload(T data)` | Re-enqueue (no load-counter increment) | | | `delete(MagazineData)` | Remove a specific record | +| | `deleteAll(Collection>)` | Remove a batch in one round trip | | | `getMetaData()` | Retrieve per-shard counters & pointers | | | `peek(Map>)` | Read without consuming | | `MagazineManager` | `refresh(List>)` | Register / update magazines | diff --git a/docs/docs/concepts/api-reference.md b/docs/docs/concepts/api-reference.md index 665013a..fa6cbf5 100644 --- a/docs/docs/concepts/api-reference.md +++ b/docs/docs/concepts/api-reference.md @@ -32,6 +32,7 @@ Magazine magazine = Magazine.builder() | `fire()` | `MagazineData` | Dequeue and return the next item. **At-most-once** — see [Delivery Semantics](delivery-semantics.md). | | `reload(T data)` | `boolean` | Re-enqueue data (decrements fire counter, not increment load counter). | | `delete(MagazineData magazineData)` | `void` | Delete a specific record from the backend. | +| `deleteAll(Collection> magazineData)` | `void` | Delete a batch of records | | `getMetaData()` | `Map` | Per-shard metadata (counters and pointers). | | `getShards()` | `int` | This magazine's **persisted** shard count (from the resolved `MagazineContext`), not the shard count configured on the storage. | | `peek(Map> shardPointersMap)` | `Set>` | Read specific records without consuming. | @@ -99,6 +100,7 @@ There is no `shards` constructor parameter — shard count is a property of the | `fire(MagazineContext context)` | `MagazineData` | Consume the next item from the context's magazine. | | `getMetaData(MagazineContext context)` | `Map` | Read per-shard metadata. | | `delete(MagazineContext context, MagazineData magazineData)` | `void` | Delete a specific record. | +| `deleteAll(MagazineContext context, Collection> magazineData)` | `void` | Delete a batch. **Abstract — every backend must implement it.** Batch it into one round trip if the backend can; otherwise loop `delete`. | | `peek(MagazineContext context, Map> shardPointersMap)` | `Set>` | Read without consuming. | Every method except `initialize` takes the `MagazineContext` as its first argument. diff --git a/docs/docs/concepts/metrics.md b/docs/docs/concepts/metrics.md index 2d098d1..d447e6f 100644 --- a/docs/docs/concepts/metrics.md +++ b/docs/docs/concepts/metrics.md @@ -117,7 +117,8 @@ All meters are tagged `magazine`. `claim_fire_pointer`, `increment_load_pointer`, `increment_load_counter`, `increment_fire_counter`, `decrement_fire_counter`, `batch_read_metadata`, `refresh_active_shards`, `read_data`, `write_data`, -`delete_data`, `batch_read_data`, `claim_dedupe_marker`, `withdraw_dedupe_marker`. +`delete_data`, `batch_delete_data`, `batch_read_data`, `claim_dedupe_marker`, +`withdraw_dedupe_marker`. `refresh_active_shards` is the batch read behind an active-shard cache refresh, split out from `batch_read_metadata` so refresh traffic is attributable without a meter of its own. A magazine on @@ -135,6 +136,8 @@ Divide `magazine.aerospike.calls` by the matching outcome counter. Healthy stead | `load()` with dedupe | **4** | + claim marker | | `reload()` | **3** | allocate pointer, write payload, decrement fire counter | | Shard discovery | 1–2 batches / `activeShardRefreshSeconds` / magazine | `refresh_active_shards`; fans out to every shard | +| `delete()` | **1** | `delete_data` | +| `deleteAll()` | **1** | `batch_delete_data`, whatever the batch size. A one-record batch takes the single-key path and counts as `delete_data` | **Calls per `fire()` does not rise with consumer count.** The claim is a guarded atomic increment, so concurrent consumers get distinct pointers and no claim is ever lost. If you see the ratio climb diff --git a/docs/docs/index.md b/docs/docs/index.md index 5fee5a5..80b5bc4 100644 --- a/docs/docs/index.md +++ b/docs/docs/index.md @@ -95,7 +95,7 @@ graph TD | Component | Role | |-----------|------| | **`MagazineManager`** | Facade for managing multiple `Magazine` instances by identifier. | -| **`Magazine`** | Type-safe wrapper providing `load`, `fire`, `reload`, `delete`, `peek`, `getMetaData`, `getShards`. | +| **`Magazine`** | Type-safe wrapper providing `load`, `fire`, `reload`, `delete`, `deleteAll`, `peek`, `getMetaData`, `getShards`. | | **`BaseMagazineStorage`** | Abstract storage contract; implemented by each backend. `initialize(String)` resolves a `MagazineContext`. | | **`MagazineContext`** | Immutable per-magazine resolved configuration (identifier, schema version, shard count) threaded through every storage call. | | **`AerospikeStorage`** | Production-ready Aerospike backend with sharding, bounded retries, and lock-free de-duplication. | diff --git a/docs/docs/upgrading.md b/docs/docs/upgrading.md index e118dcf..2740a57 100644 --- a/docs/docs/upgrading.md +++ b/docs/docs/upgrading.md @@ -122,6 +122,8 @@ classpath. Declare it directly if you depended on it transitively. [metrics](concepts/metrics.md). - `activeShardRefreshSeconds` — tune shard-discovery read load. - `metadataCacheSeconds` on the bundle — throttle dashboard fan-out. +- **`Magazine.deleteAll(Collection>)`** — retire a whole batch in one round trip. + Optional for callers; **mandatory for anyone implementing `BaseMagazineStorage`**, see section 4. ## Checklist @@ -133,3 +135,4 @@ classpath. Declare it directly if you depended on it transitively. 5. Register auth for peek, or accept it returning 403. 6. Update anything parsing the dashboard JSON for numbers-as-strings. 7. Declare Guava or DLM directly if you were getting them transitively. +8. If you implement `BaseMagazineStorage`, implement the new abstract `deleteAll`. diff --git a/docs/docs/usage.md b/docs/docs/usage.md index 7a61efc..73c1f1a 100644 --- a/docs/docs/usage.md +++ b/docs/docs/usage.md @@ -10,6 +10,7 @@ Every `Magazine` instance exposes these operations: | `fire()` | Dequeue and return the next item. | | `reload(T data)` | Re-enqueue data (decrements fire counter instead of incrementing load counter). | | `delete(MagazineData)` | Remove a specific record from the backend. | +| `deleteAll(Collection>)` | Remove a batch of records in one round trip. | | `getMetaData()` | Retrieve per-shard counters and pointers. | | `getShards()` | This magazine's persisted shard count. | | `peek(Map>)` | Read specific shard/pointer records without consuming. | @@ -103,6 +104,10 @@ magazine.delete(fired); !!! warning `delete()` throws a `MagazineException` with `INVALID_CONFIGURATION` if the supplied `MagazineData` belongs to a different magazine than the one you call it on. Do not route records returned by one magazine into another's `delete()`. + `deleteAll()` applies the same rule to every record in the batch, and validates the whole batch + before deleting any of it - so a batch carrying one foreign record is rejected without + retiring the records it did own. + ## Peeking Data Read records from specific shards and pointers **without modifying any counters or pointers**. Useful for inspection, debugging, or replay scenarios. diff --git a/magazine-core/src/main/java/com/phonepe/magazine/Magazine.java b/magazine-core/src/main/java/com/phonepe/magazine/Magazine.java index 6da15a4..8d8860e 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/Magazine.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/Magazine.java @@ -23,6 +23,7 @@ import com.phonepe.magazine.entity.MetaData; import com.phonepe.magazine.exception.MagazineExceptions; import java.time.Instant; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; @@ -102,6 +103,19 @@ public void delete(final MagazineData magazineData) { baseMagazineStorage.delete(context, magazineData); } + /** + * Delete a batch of records that have all been handled, in one round trip where the backend + * supports it. + * + * @param magazineData The MagazineData records to be deleted. Empty is a no-op. + * @throws com.phonepe.magazine.exception.MagazineException with {@code INVALID_CONFIGURATION} + * if any record belongs to a different magazine. The batch is validated before any of + * it is deleted. + */ + public void deleteAll(final Collection> magazineData) { + baseMagazineStorage.deleteAll(context, magazineData); + } + /** * Retrieve metadata of the specified magazine i.e the number of loaded or fired, pointers and counters. * diff --git a/magazine-core/src/main/java/com/phonepe/magazine/core/BaseMagazineStorage.java b/magazine-core/src/main/java/com/phonepe/magazine/core/BaseMagazineStorage.java index b85cedd..1dc9813 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/core/BaseMagazineStorage.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/core/BaseMagazineStorage.java @@ -25,6 +25,7 @@ import com.phonepe.magazine.exception.ErrorCode; import com.phonepe.magazine.exception.MagazineExceptions; import java.time.Instant; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; @@ -132,6 +133,14 @@ public abstract boolean reload( */ public abstract void delete(final MagazineContext context, final MagazineData magazineData); + /** + * Delete a batch of records that have all been handled. + * + * @param context The magazine operation context. + * @param magazineData The records to delete. An empty collection is a no-op. + */ + public abstract void deleteAll(final MagazineContext context, final Collection> magazineData); + /** * Peek data from specific shards and pointers within the magazine. * diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java index 39f629c..e9b0803 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java @@ -41,6 +41,7 @@ import io.micrometer.core.instrument.Metrics; import io.micrometer.core.instrument.composite.CompositeMeterRegistry; import java.time.Instant; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; @@ -217,16 +218,28 @@ public Map getMetaData(final MagazineContext context) { @Override public void delete(final MagazineContext context, final MagazineData magazineData) { + validateOwnership(context, magazineData); + try { + dataStore.delete(context, magazineData); + } catch (Exception e) { + throw mapFailure(e, ErrorMessage.ERROR_DELETING_DATA, context); + } + } + + /** + * Every record is validated before any is deleted, so a batch carrying one foreign record is + * rejected whole rather than half applied. + */ + @Override + public void deleteAll(final MagazineContext context, final Collection> magazineData) { if (Objects.isNull(magazineData)) { throw MagazineExceptions.invalidConfiguration("Magazine data is required."); } - if (!context.getMagazineIdentifier().equals(magazineData.getMagazineIdentifier())) { - throw MagazineExceptions.invalidConfiguration("Magazine data belongs to a different magazine."); - } + magazineData.forEach(data -> validateOwnership(context, data)); try { - dataStore.delete(context, magazineData); + dataStore.deleteAll(context, magazineData); } catch (Exception e) { - throw mapFailure(e, ErrorMessage.ERROR_DELETING_DATA, context); + throw mapFailure(e, ErrorMessage.ERROR_BATCH_DELETING_DATA, context); } } @@ -368,6 +381,15 @@ private void validateDataType(final T data) { } } + private void validateOwnership(final MagazineContext context, final MagazineData magazineData) { + if (Objects.isNull(magazineData)) { + throw MagazineExceptions.invalidConfiguration("Magazine data is required."); + } + if (!context.getMagazineIdentifier().equals(magazineData.getMagazineIdentifier())) { + throw MagazineExceptions.invalidConfiguration("Magazine data belongs to a different magazine."); + } + } + /** * Normalises anything thrown on a storage path into a {@link MagazineException}. *

diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikePolicies.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikePolicies.java index 8964bb1..40c460a 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikePolicies.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikePolicies.java @@ -17,6 +17,8 @@ package com.phonepe.magazine.impl.aerospike.common; import com.aerospike.client.IAerospikeClient; +import com.aerospike.client.policy.BatchDeletePolicy; +import com.aerospike.client.policy.BatchPolicy; import com.aerospike.client.policy.WritePolicy; import java.util.Objects; import lombok.AccessLevel; @@ -38,4 +40,16 @@ public static WritePolicy writePolicy(final IAerospikeClient client) { final WritePolicy defaults = client.getWritePolicyDefault(); return Objects.isNull(defaults) ? new WritePolicy() : new WritePolicy(defaults); } + + /** @return a copy of the client's default batch policy, or a fresh default. See above. */ + public static BatchPolicy batchPolicy(final IAerospikeClient client) { + final BatchPolicy defaults = client.getBatchPolicyDefault(); + return Objects.isNull(defaults) ? new BatchPolicy() : new BatchPolicy(defaults); + } + + /** @return a copy of the client's default batch delete policy, or a fresh default. See above. */ + public static BatchDeletePolicy batchDeletePolicy(final IAerospikeClient client) { + final BatchDeletePolicy defaults = client.getBatchDeletePolicyDefault(); + return Objects.isNull(defaults) ? new BatchDeletePolicy() : new BatchDeletePolicy(defaults); + } } diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/ErrorMessage.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/ErrorMessage.java index 4f2f11d..142ef29 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/ErrorMessage.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/ErrorMessage.java @@ -32,6 +32,8 @@ public final class ErrorMessage { public static final String ERROR_READING_COUNTERS = "Error reading counters [magazineIdentifier = %s]"; public static final String NO_DATA_TO_FIRE = "No data to fire [magazineIdentifier = %s]"; public static final String ERROR_DELETING_DATA = "Error deleting data [magazineIdentifier = %s]"; + public static final String ERROR_BATCH_DELETING_DATA = + "Error batch deleting data [magazineIdentifier = %s]"; public static final String ERROR_INITIALIZING_MAGAZINE = "Error initializing magazine [magazineIdentifier = %s]"; public static final String ERROR_PEEKING_DATA = "Error peeking data [magazineIdentifier = %s]"; diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineDataStore.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineDataStore.java index 5e4a685..aaef9c5 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineDataStore.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineDataStore.java @@ -17,11 +17,15 @@ package com.phonepe.magazine.impl.aerospike.store; import com.aerospike.client.BatchRead; +import com.aerospike.client.BatchRecord; +import com.aerospike.client.BatchResults; import com.aerospike.client.Bin; import com.aerospike.client.IAerospikeClient; import com.aerospike.client.Key; import com.aerospike.client.Record; import com.aerospike.client.ResultCode; +import com.aerospike.client.policy.BatchDeletePolicy; +import com.aerospike.client.policy.BatchPolicy; import com.aerospike.client.policy.WritePolicy; import com.phonepe.magazine.entity.MagazineContext; import com.phonepe.magazine.entity.MagazineData; @@ -34,6 +38,7 @@ import com.phonepe.magazine.metrics.MagazineMetrics; import com.phonepe.magazine.metrics.StorageOperation; import java.util.ArrayList; +import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -57,6 +62,8 @@ public final class MagazineDataStore { private final Class clazz; private final WritePolicy writePolicy; private final WritePolicy deletePolicy; + private final BatchPolicy batchPolicy; + private final BatchDeletePolicy batchDeletePolicy; public MagazineDataStore(final IAerospikeClient client, final AerospikeRetryer retryerFactory, @@ -76,6 +83,8 @@ public MagazineDataStore(final IAerospikeClient client, this.writePolicy.expiration = recordTtl; this.writePolicy.sendKey = true; this.deletePolicy = AerospikePolicies.writePolicy(client); + this.batchPolicy = AerospikePolicies.batchPolicy(client); + this.batchDeletePolicy = AerospikePolicies.batchDeletePolicy(client); } public boolean write(final MagazineContext context, @@ -108,6 +117,33 @@ public void delete(final MagazineContext context, final MagazineData magazine }); } + public void deleteAll(final MagazineContext context, final Collection> magazineData) { + if (magazineData.isEmpty()) { + return; + } + if (magazineData.size() == 1) { + delete(context, magazineData.iterator().next()); + return; + } + final Key[] keys = magazineData.stream() + .map(data -> dataKey(context, data.getShard(), data.getFirePointer())) + .toArray(Key[]::new); + final BatchResults results = retryerFactory.call(() -> { + metrics.aerospikeCall(context.getMagazineIdentifier(), StorageOperation.BATCH_DELETE_DATA); + return client.delete(batchPolicy, batchDeletePolicy, keys); + }); + if (results.status) { + return; + } + for (BatchRecord result : results.records) { + if (result.resultCode != ResultCode.OK && result.resultCode != ResultCode.KEY_NOT_FOUND_ERROR) { + throw MagazineExceptions.connectionError( + String.format(ErrorMessage.ERROR_BATCH_DELETING_DATA, context.getMagazineIdentifier()), + null); + } + } + } + public Set> peek(final MagazineContext context, final Map> shardPointersMap) { final List requests = new ArrayList<>(); diff --git a/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java b/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java index c09ed40..641fabf 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java @@ -51,6 +51,7 @@ public enum StorageOperation { WRITE_DATA("write_data"), DELETE_DATA("delete_data"), BATCH_READ_DATA("batch_read_data"), + BATCH_DELETE_DATA("batch_delete_data"), /** The checkpoint write; one per shard per window, not one per claim. */ WRITE_FIRE_HISTORY("write_fire_history"), /** The batch read behind {@code firePointerBefore}. */ diff --git a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineLifecycleTest.java b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineLifecycleTest.java index 48d9509..1d164d5 100644 --- a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineLifecycleTest.java +++ b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineLifecycleTest.java @@ -28,6 +28,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Set; @@ -128,6 +129,94 @@ void magazineCannotDeleteAnotherMagazinesData() { assertMagazineError(ErrorCode.INVALID_CONFIGURATION, () -> first.delete(magazineData)); } + @Test + void batchDeleteRetiresEveryRecordInTheBatch() { + Magazine magazine = Magazine.builder() + .magazineIdentifier("BATCH_DELETE_MAGAZINE") + .baseMagazineStorage(buildMagazineStorage(String.class, false)) + .build(); + List> fired = new ArrayList<>(); + for (int i = 0; i < 5; i++) { + assertTrue(magazine.load("DATA" + i)); + } + for (int i = 0; i < 5; i++) { + fired.add(magazine.fire()); + } + fired.forEach(data -> assertNotNull(aerospikeClient.get(aerospikeClient.getWritePolicyDefault(), + new Key("NAMESPACE", "FARM_ID_DATA_SET", dataKey(data, 16))))); + + magazine.deleteAll(fired); + + fired.forEach(data -> assertNull(aerospikeClient.get(aerospikeClient.getWritePolicyDefault(), + new Key("NAMESPACE", "FARM_ID_DATA_SET", dataKey(data, 16))))); + } + + @Test + void batchDeleteOfASingleRecordRetiresIt() { + Magazine magazine = Magazine.builder() + .magazineIdentifier("BATCH_DELETE_SINGLE_MAGAZINE") + .baseMagazineStorage(buildMagazineStorage(String.class, false)) + .build(); + assertTrue(magazine.load("DATA")); + MagazineData fired = magazine.fire(); + + magazine.deleteAll(List.of(fired)); + + assertNull(aerospikeClient.get(aerospikeClient.getWritePolicyDefault(), + new Key("NAMESPACE", "FARM_ID_DATA_SET", dataKey(fired, 16)))); + } + + @Test + void batchDeleteToleratesARecordThatIsAlreadyGone() { + Magazine magazine = Magazine.builder() + .magazineIdentifier("BATCH_DELETE_IDEMPOTENT_MAGAZINE") + .baseMagazineStorage(buildMagazineStorage(String.class, false)) + .build(); + assertTrue(magazine.load("DATA1")); + assertTrue(magazine.load("DATA2")); + MagazineData first = magazine.fire(); + MagazineData second = magazine.fire(); + magazine.delete(first); + + // the batch still carries the record that has already been retired + assertDoesNotThrow(() -> magazine.deleteAll(List.of(first, second))); + + assertNull(aerospikeClient.get(aerospikeClient.getWritePolicyDefault(), + new Key("NAMESPACE", "FARM_ID_DATA_SET", dataKey(second, 16)))); + } + + @Test + void batchDeleteOfAnEmptyBatchIsANoOp() { + Magazine magazine = Magazine.builder() + .magazineIdentifier("BATCH_DELETE_EMPTY_MAGAZINE") + .baseMagazineStorage(buildMagazineStorage(String.class, false)) + .build(); + + assertDoesNotThrow(() -> magazine.deleteAll(List.>of())); + } + + @Test + void batchDeleteRejectsTheWholeBatchWhenOneRecordIsForeign() { + Magazine first = Magazine.builder() + .magazineIdentifier("BATCH_DELETE_OWNER_MAGAZINE") + .baseMagazineStorage(buildMagazineStorage(String.class, false)) + .build(); + Magazine second = Magazine.builder() + .magazineIdentifier("BATCH_DELETE_OTHER_MAGAZINE") + .baseMagazineStorage(buildMagazineStorage(String.class, false)) + .build(); + assertTrue(first.load("MINE")); + assertTrue(second.load("THEIRS")); + MagazineData mine = first.fire(); + MagazineData theirs = second.fire(); + + assertMagazineError(ErrorCode.INVALID_CONFIGURATION, () -> first.deleteAll(List.of(mine, theirs))); + + // validation runs before any delete, so the owned record must survive the rejection + assertNotNull(aerospikeClient.get(aerospikeClient.getWritePolicyDefault(), + new Key("NAMESPACE", "FARM_ID_DATA_SET", dataKey(mine, 16)))); + } + @Test void magazinePeekTest() { Magazine magazine = magazineManager.getMagazine("MAGAZINE_ID1"); diff --git a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineMetricsTest.java b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineMetricsTest.java index 7cf8f2a..d36563f 100644 --- a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineMetricsTest.java +++ b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineMetricsTest.java @@ -17,8 +17,11 @@ package com.phonepe.magazine.impl.aerospike; import com.phonepe.magazine.entity.MagazineContext; +import com.phonepe.magazine.entity.MagazineData; import com.phonepe.magazine.entity.MagazineScope; import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import java.util.ArrayList; +import java.util.List; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -67,6 +70,53 @@ void hotPathRoundTripCountsAreBounded() { assertEquals(1.0, counterValue(registry, "magazine.fire.outcomes", "outcome", "delivered")); } + @Test + void batchDeleteCostsOneRoundTripWhateverTheBatchSize() { + SimpleMeterRegistry registry = new SimpleMeterRegistry(); + AerospikeStorage storage = buildStorage( + buildStorageConfig("NAMESPACE", "DATA_SET", "META_SET", + 30 * 24 * 60 * 60, 2 * 30 * 24 * 60 * 60, 1), + String.class, false, "FARM_ID", "CLIENT_ID", MagazineScope.LOCAL, aerospikeClient, + registry); + MagazineContext context = storage.initialize("BATCH_DELETE_ROUND_TRIP_MAGAZINE"); + List> fired = new ArrayList<>(); + for (int i = 0; i < 5; i++) { + assertTrue(storage.load(context, "PAYLOAD" + i)); + } + for (int i = 0; i < 5; i++) { + fired.add(storage.fire(context)); + } + + storage.deleteAll(context, fired); + + assertEquals(1.0, counterValue(registry, "magazine.aerospike.calls", + "operation", "batch_delete_data")); + assertEquals(0.0, counterValue(registry, "magazine.aerospike.calls", + "operation", "delete_data")); + } + + /** + * A one-record batch takes the single-key path, because batch framing buys nothing when there + * is nothing to amortise it over. + */ + @Test + void aSingleRecordBatchUsesTheSingleKeyPath() { + SimpleMeterRegistry registry = new SimpleMeterRegistry(); + AerospikeStorage storage = buildStorage( + buildStorageConfig("NAMESPACE", "DATA_SET", "META_SET", + 30 * 24 * 60 * 60, 2 * 30 * 24 * 60 * 60, 1), + String.class, false, "FARM_ID", "CLIENT_ID", MagazineScope.LOCAL, aerospikeClient, + registry); + MagazineContext context = storage.initialize("SINGLE_BATCH_ROUND_TRIP_MAGAZINE"); + assertTrue(storage.load(context, "PAYLOAD")); + + storage.deleteAll(context, List.of(storage.fire(context))); + + assertEquals(1.0, counterValue(registry, "magazine.aerospike.calls", "operation", "delete_data")); + assertEquals(0.0, counterValue(registry, "magazine.aerospike.calls", + "operation", "batch_delete_data")); + } + @Test void metricsCanBeDisabledExplicitlyEvenWithARegistry() { SimpleMeterRegistry registry = new SimpleMeterRegistry(); From bae5bf39ed48787ce59fc243f107d1d1e0ffca9f Mon Sep 17 00:00:00 2001 From: Shantanu Tiwari Date: Wed, 23 Sep 2026 17:14:18 +0530 Subject: [PATCH 05/10] Fix sonar smells --- .../impl/aerospike/AerospikeStorage.java | 5 +- .../store/MagazineMetadataStore.java | 21 ++++---- .../aerospike/MagazineFireHistoryTest.java | 52 +++++++++++-------- 3 files changed, 44 insertions(+), 34 deletions(-) diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java index e9b0803..9e444df 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java @@ -105,8 +105,9 @@ public AerospikeStorage( this.initializer = new AerospikeMagazineInitializer(aerospikeClient, retryerFactory, namespace, metaSetName, storageConfig.getShards(), storageConfig.isAllowShardIncrease()); this.metadataStore = new MagazineMetadataStore(aerospikeClient, retryerFactory, metrics, - namespace, metaSetName, getMetaDataTtl(), storageConfig.isFireHistoryEnabled(), - storageConfig.fireHistoryWindowMillis(), storageConfig.getFireHistoryEntries()); + namespace, metaSetName, getMetaDataTtl(), + new MagazineMetadataStore.FireHistoryConfig(storageConfig.isFireHistoryEnabled(), + storageConfig.fireHistoryWindowMillis(), storageConfig.getFireHistoryEntries())); this.dataStore = new MagazineDataStore<>(aerospikeClient, retryerFactory, metrics, namespace, dataSetName, getRecordTtl(), clazz); this.activeShards = new ActiveShardSelector(this::loadActiveShards, diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java index 3826894..3d63943 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java @@ -115,23 +115,24 @@ public final class MagazineMetadataStore { */ private final Map checkpointWindows = new ConcurrentHashMap<>(); + public record FireHistoryConfig(boolean enabled, long windowMillis, int entries) { + } + public MagazineMetadataStore(final IAerospikeClient client, final AerospikeRetryer retryerFactory, final MagazineMetrics metrics, final String namespace, final String metaSetName, final int metaDataTtl, - final boolean fireHistoryEnabled, - final long fireHistoryWindowMillis, - final int fireHistoryEntries) { + final FireHistoryConfig fireHistoryConfig) { this.client = client; this.retryerFactory = retryerFactory; this.metrics = metrics; this.namespace = namespace; this.metaSetName = metaSetName; - this.fireHistoryEnabled = fireHistoryEnabled; - this.fireHistoryWindowMillis = fireHistoryWindowMillis; - this.fireHistoryEntries = fireHistoryEntries; + this.fireHistoryEnabled = fireHistoryConfig.enabled(); + this.fireHistoryWindowMillis = fireHistoryConfig.windowMillis(); + this.fireHistoryEntries = fireHistoryConfig.entries(); this.claimPolicy = AerospikePolicies.writePolicy(client); this.claimPolicy.recordExistsAction = RecordExistsAction.UPDATE_ONLY; @@ -346,10 +347,10 @@ private Record[] readFireHistory(final MagazineContext context) { } private Optional resolveCheckpoint(final MagazineContext context, - final Record record, + final Record pointerRecord, final Instant instant, final int shard) { - final SortedMap history = readHistory(record); + final SortedMap history = readHistory(pointerRecord); final long cutoff = instant.toEpochMilli(); for (Map.Entry entry : history.entrySet()) { final long windowStart = -entry.getKey(); @@ -369,8 +370,8 @@ private Optional resolveCheckpoint(final MagazineContext context } @SuppressWarnings("unchecked") - private SortedMap readHistory(final Record record) { - final Object raw = Objects.isNull(record) ? null : record.getValue(AerospikeConstants.FIRE_HISTORY); + private SortedMap readHistory(final Record pointerRecord) { + final Object raw = Objects.isNull(pointerRecord) ? null : pointerRecord.getValue(AerospikeConstants.FIRE_HISTORY); return raw instanceof Map ? new TreeMap<>((Map) raw) : new TreeMap<>(); } diff --git a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java index 9096d3b..74f04f3 100644 --- a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java +++ b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java @@ -88,13 +88,13 @@ void recordsOneCheckpointPerWindow() { @Test @DisplayName("a new window adds a checkpoint without disturbing the old one") - void newWindowAddsACheckpoint() throws Exception { + void newWindowAddsACheckpoint() { final Magazine magazine = magazine("HIST_ROLL", "HIST_ROLL_SET", true, 1, 8); loadAndFire(magazine, 2); magazine.firePointerBefore(Instant.now()); final Map first = new TreeMap<>(history(magazine, 0)); - Thread.sleep(1_100); + awaitNextWindow(); loadAndFire(magazine, 2); magazine.firePointerBefore(Instant.now()); @@ -108,13 +108,13 @@ void newWindowAddsACheckpoint() throws Exception { @Test @DisplayName("history is capped by count, so it cannot grow without bound") - void historyIsBoundedByCount() throws Exception { + void historyIsBoundedByCount() { final int cap = 3; final Magazine magazine = magazine("HIST_CAP", "HIST_CAP_SET", true, 1, cap); for (int round = 0; round < cap + 3; round++) { loadAndFire(magazine, 1); magazine.firePointerBefore(Instant.now()); - Thread.sleep(1_100); + awaitNextWindow(); } assertEquals(cap, history(magazine, 0).size(), "the newest entries are kept and everything past the cap is dropped"); @@ -127,22 +127,22 @@ void checkpointsNeverOverReport() { loadAndFire(magazine, 5); magazine.firePointerBefore(Instant.now()); - final Record record = record(magazine, 0); - final long actual = record.getLong(AerospikeConstants.FIRE_POINTER); + final Record pointerRecord = metadataRecord(magazine, 0); + final long actual = pointerRecord.getLong(AerospikeConstants.FIRE_POINTER); history(magazine, 0).values().forEach(recorded -> assertTrue(recorded <= actual, "checkpoint " + recorded + " must not exceed the fire pointer " + actual)); } @Test @DisplayName("the answer is the newest checkpoint at or before the instant asked about") - void returnsNewestCheckpointNotAfterTheInstant() throws Exception { + void returnsNewestCheckpointNotAfterTheInstant() { final Magazine magazine = magazine("HIST_PICK", "HIST_PICK_SET", true, 1, 8); loadAndFire(magazine, 2); magazine.firePointerBefore(Instant.now()); final Instant afterFirstBurst = Instant.now(); - final long pointerAfterFirstBurst = record(magazine, 0).getLong(AerospikeConstants.FIRE_POINTER); + final long pointerAfterFirstBurst = metadataRecord(magazine, 0).getLong(AerospikeConstants.FIRE_POINTER); - Thread.sleep(1_100); + awaitNextWindow(); loadAndFire(magazine, 4); magazine.firePointerBefore(Instant.now()); @@ -167,14 +167,14 @@ void youngHistoryReturnsAbsent() { @Test @DisplayName("a full history that cannot reach back far enough is a configuration error, not an empty answer") - void evictedHistoryFailsLoudly() throws Exception { + void evictedHistoryFailsLoudly() { final int cap = 2; final Magazine magazine = magazine("HIST_EVICTED", "HIST_EVICTED_SET", true, 1, cap); final Instant longAgo = Instant.now().minus(Duration.ofHours(1)); for (int round = 0; round < cap + 1; round++) { loadAndFire(magazine, 1); magazine.firePointerBefore(Instant.now()); - Thread.sleep(1_100); + awaitNextWindow(); } // At capacity with every retained entry newer than the question: real history was @@ -186,13 +186,13 @@ void evictedHistoryFailsLoudly() throws Exception { @Test @DisplayName("reading advances history, so a burst that is never fired from again stays reachable") - void readingRecordsACheckpoint() throws Exception { + void readingRecordsACheckpoint() { final Magazine magazine = magazine("HIST_BURST", "HIST_BURST_SET", true, 1, 8); loadAndFire(magazine, 3); magazine.firePointerBefore(Instant.now()); final long newestBefore = newestWindowKey(magazine); - Thread.sleep(1_100); + awaitNextWindow(); // No load, no fire: the magazine is idle. Reading alone must still move history forward, // otherwise a magazine that took one burst and was then abandoned would hold a single // checkpoint predating the burst and its orphans would never become visible. @@ -250,12 +250,12 @@ void lateShardIsCheckpointedInTheSameWindow() { @Test @DisplayName("the whole retained history reads back newest first, and reading it records nothing") - void fireHistoryReturnsEverythingRetainedWithoutRecording() throws Exception { + void fireHistoryReturnsEverythingRetainedWithoutRecording() { final Magazine magazine = magazine("HIST_ALL", "HIST_ALL_SET", true, 1, 8); for (int round = 0; round < 3; round++) { loadAndFire(magazine, 1); magazine.firePointerBefore(Instant.now()); - Thread.sleep(1_100); + awaitNextWindow(); } final List checkpoints = magazine.fireHistory().get("SHARD_0"); @@ -270,7 +270,7 @@ void fireHistoryReturnsEverythingRetainedWithoutRecording() throws Exception { // A console browsing history must not write to the magazine it is inspecting. final int before = history(magazine, 0).size(); - Thread.sleep(1_100); + awaitNextWindow(); magazine.fireHistory(); assertEquals(before, history(magazine, 0).size(), "reading history must not record a window"); } @@ -343,6 +343,13 @@ private AerospikeStorage storageWithHistory(final String setName, final String.class, false, "FARM_ID", "CLIENT_ID", MagazineScope.LOCAL, aerospikeClient); } + private void awaitNextWindow() { + Awaitility.await("the current fire-history window to close") + .pollDelay(Duration.ofMillis(1_100)) + .atMost(Duration.ofSeconds(5)) + .until(() -> true); + } + private void loadAndFire(final Magazine magazine, final int count) { for (int i = 0; i < count; i++) { magazine.load("message-" + random.nextLong()); @@ -371,7 +378,7 @@ private void fireWhenVisible(final Magazine magazine) { }); } - private Record record(final Magazine magazine, final int shard) { + private Record metadataRecord(final Magazine magazine, final int shard) { final String name = magazine.getShards() <= 1 ? magazine.getMagazineIdentifier() + "_" + AerospikeConstants.METADATA : magazine.getMagazineIdentifier() + "_SHARD_" + shard + "_" + AerospikeConstants.METADATA; @@ -380,18 +387,18 @@ private Record record(final Magazine magazine, final int shard) { @SuppressWarnings("unchecked") private Map history(final Magazine magazine, final int shard) { - final Record record = record(magazine, shard); - if (record == null) { + final Record pointerRecord = metadataRecord(magazine, shard); + if (pointerRecord == null) { return null; } - final Object raw = record.getValue(AerospikeConstants.FIRE_HISTORY); + final Object raw = pointerRecord.getValue(AerospikeConstants.FIRE_HISTORY); return raw instanceof Map ? (Map) raw : null; } private int shardsWithMetadata(final Magazine magazine, final int shards) { int found = 0; for (int shard = 0; shard < shards; shard++) { - if (record(magazine, shard) != null) { + if (metadataRecord(magazine, shard) != null) { found++; } } @@ -408,7 +415,8 @@ private long newestWindowKey(final Magazine magazine) { return history.keySet().stream().mapToLong(Long::longValue).min().orElse(0L); } - private int totalCheckpoints(final Magazine magazine, final int shards) { int total = 0; + private int totalCheckpoints(final Magazine magazine, final int shards) { + int total = 0; for (int shard = 0; shard < shards; shard++) { final Map history = history(magazine, shard); total += history == null ? 0 : history.size(); From d4c6c902cdec869a3a5072b99b258cd7a5ea7416 Mon Sep 17 00:00:00 2001 From: Shantanu Tiwari Date: Wed, 23 Sep 2026 18:55:10 +0530 Subject: [PATCH 06/10] Fix MagazineManager evicting every other magazine on registration --- README.md | 18 +- docs/docs/concepts/api-reference.md | 8 +- docs/docs/concepts/metrics.md | 2 +- docs/docs/dropwizard-bundle.md | 4 +- docs/docs/usage.md | 22 +- magazine-core/pom.xml | 2 +- .../com/phonepe/magazine/MagazineManager.java | 121 +++++++- .../phonepe/magazine/MagazineManagerTest.java | 271 ++++++++++++++++++ .../impl/aerospike/MagazineLifecycleTest.java | 2 +- magazine-coverage/pom.xml | 2 +- magazine-dw-bundle/pom.xml | 2 +- .../MagazineDashboardDemoApplication.java | 2 +- pom.xml | 2 +- 13 files changed, 431 insertions(+), 27 deletions(-) create mode 100644 magazine-core/src/test/java/com/phonepe/magazine/MagazineManagerTest.java diff --git a/README.md b/README.md index 2f072ae..05e0396 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,17 @@ Set> peeked = magazine.peek(shardPointers); ```java MagazineManager manager = new MagazineManager("my-service"); -manager.refresh(List.of(magazine)); + +// Add one magazine, leaving any others registered +manager.register(magazine); + +// Or build it only if it is not already registered - constructing a Magazine +// costs a shard-configuration read, so prefer this on a per-request path +Magazine notifications = manager.getOrRegister("notifications", + () -> Magazine.builder() + .baseMagazineStorage(storage) + .magazineIdentifier("notifications") + .build()); // Retrieve by identifier Magazine m = manager.getMagazine("notifications"); @@ -177,7 +187,11 @@ For peek, try shard `0` pointers `35` and `36` on `email-jobs`, or pointers `50` | | `deleteAll(Collection>)` | Remove a batch in one round trip | | | `getMetaData()` | Retrieve per-shard counters & pointers | | | `peek(Map>)` | Read without consuming | -| `MagazineManager` | `refresh(List>)` | Register / update magazines | +| `MagazineManager` | `register(Magazine)` | Add one magazine, keeping the rest | +| | `getOrRegister(String, Supplier>)` | Look up, building only if absent | +| | `find(String)` | Look up, empty if absent | +| | `replaceAll(List>)` | Replace the whole registration set | +| | `unregister(String)` | Drop one registration | | | `getMagazine(String)` | Retrieve magazine by identifier | ## Documentation diff --git a/docs/docs/concepts/api-reference.md b/docs/docs/concepts/api-reference.md index fa6cbf5..f7b6aa7 100644 --- a/docs/docs/concepts/api-reference.md +++ b/docs/docs/concepts/api-reference.md @@ -63,8 +63,12 @@ MagazineManager manager = new MagazineManager("my-client-id"); | Method | Returns | Description | |--------|---------|-------------| -| `refresh(List> magazines)` | `void` | Atomically replace all registered magazines. | +| `register(Magazine magazine)` | `void` | Add one magazine, leaving other registrations untouched. Replaces any registration under the same identifier. | +| `getOrRegister(String magazineIdentifier, Supplier> factory)` | `Magazine` | Return the registered magazine, invoking `factory` only if absent. Avoids the shard-configuration read a redundant build would cost. | +| `replaceAll(List> magazines)` | `void` | Atomically replace all registered magazines. Anything absent is evicted. | +| `unregister(String magazineIdentifier)` | `boolean` | Drop one registration. Storage is untouched. | | `getMagazine(String magazineIdentifier)` | `Magazine` | Retrieve a magazine by identifier. Throws `MagazineException` with `MAGAZINE_NOT_FOUND` if not found. | +| `find(String magazineIdentifier)` | `Optional>` | Retrieve a magazine by identifier, empty if absent. Use when the caller owns the miss path. | --- @@ -172,7 +176,7 @@ Descriptive label for a storage backend. A plain enum — dispatch happens throu ## Thread Safety -- `MagazineManager` publishes an immutable map on each `refresh()`, so concurrent readers observe either the previous or new complete registration set. +- `MagazineManager` publishes an immutable map on every mutation, so concurrent readers observe either the previous or the new complete registration set. `register`, `getOrRegister` and `unregister` update it with a compare-and-set loop, so concurrent registrations cannot lose each other. - `Magazine` delegates all operations to the storage backend. Thread safety depends on the backend implementation. - `MagazineContext` is immutable and safe to share. - `AerospikeStorage` is thread-safe for all operations. There is no distributed lock: the fire claim is a guarded atomic increment that cannot be lost, and de-duplication relies on a `CREATE_ONLY` write that the server resolves. diff --git a/docs/docs/concepts/metrics.md b/docs/docs/concepts/metrics.md index d447e6f..2e16be2 100644 --- a/docs/docs/concepts/metrics.md +++ b/docs/docs/concepts/metrics.md @@ -45,7 +45,7 @@ public class MyApp extends Application { // no meterRegistry: the bundle already attached one globally .build(); - magazineBundle.getMagazineManager().refresh(List.of( + magazineBundle.getMagazineManager().replaceAll(List.of( Magazine.builder() .magazineIdentifier("email-jobs") .baseMagazineStorage(storage) diff --git a/docs/docs/dropwizard-bundle.md b/docs/docs/dropwizard-bundle.md index 962ca28..cbad196 100644 --- a/docs/docs/dropwizard-bundle.md +++ b/docs/docs/dropwizard-bundle.md @@ -158,11 +158,11 @@ public void initialize(Bootstrap bootstrap) { @Override public void run(ServiceConfiguration configuration, Environment environment) { - magazineBundle.getMagazineManager().refresh(configuredMagazines); + magazineBundle.getMagazineManager().replaceAll(configuredMagazines); } ``` -The bundle creates and exposes its `MagazineManager`, following the Ignis bundle pattern. `MagazineManager.refresh(...)` atomically replaces the registered map, and API/dashboard requests see the new magazine set immediately without a restart. +The bundle creates and exposes its `MagazineManager`, following the Ignis bundle pattern. `MagazineManager.replaceAll(...)` atomically replaces the registered map, and API/dashboard requests see the new magazine set immediately without a restart. Use `register(...)` to add a single magazine without evicting the others. `MagazineBundle` declares exactly two abstract methods, both shown above: diff --git a/docs/docs/usage.md b/docs/docs/usage.md index 73c1f1a..0aa3030 100644 --- a/docs/docs/usage.md +++ b/docs/docs/usage.md @@ -237,7 +237,7 @@ Magazine idsMag = Magazine.builder() .build(); // Register all magazines -manager.refresh(List.of(ordersMag, idsMag)); +manager.replaceAll(List.of(ordersMag, idsMag)); // Retrieve by identifier Magazine orders = manager.getMagazine("orders"); @@ -247,8 +247,24 @@ Magazine ids = manager.getMagazine("id-pool"); MagazineData id = ids.fire(); ``` -!!! note - `refresh()` atomically replaces the internal magazine map. Call it whenever your magazine topology changes (e.g. on config reload). +!!! warning "`replaceAll` evicts" + `replaceAll()` atomically replaces the internal magazine map, so **every magazine absent from the list is unregistered**. Calling it with a single magazine drops all the others, and the next `getMagazine()` for one of them throws `MAGAZINE_NOT_FOUND`. + + To add one magazine without disturbing the rest, use `register(magazine)`. On a per-request path use `getOrRegister(identifier, supplier)`: constructing a `Magazine` resolves its persisted shard configuration over the network, and `getOrRegister` skips that entirely when the handle is already held. + +If you want to own the miss path yourself, `find()` is the non-throwing lookup: + +```java +Magazine orders = manager.find("orders") + .orElseGet(() -> { + Magazine built = Magazine.builder() + .baseMagazineStorage(stringStorage) + .magazineIdentifier("orders") + .build(); + manager.register(built); + return built; + }); +``` ## De-duplication diff --git a/magazine-core/pom.xml b/magazine-core/pom.xml index a21f528..2e93619 100644 --- a/magazine-core/pom.xml +++ b/magazine-core/pom.xml @@ -7,7 +7,7 @@ com.phonepe magazine - 2.0.0-SNAPSHOT + 2.0.1-SNAPSHOT magazine-core diff --git a/magazine-core/src/main/java/com/phonepe/magazine/MagazineManager.java b/magazine-core/src/main/java/com/phonepe/magazine/MagazineManager.java index 2ace9cb..881b42b 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/MagazineManager.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/MagazineManager.java @@ -18,18 +18,25 @@ import com.phonepe.magazine.exception.ErrorCode; import com.phonepe.magazine.exception.MagazineException; +import com.phonepe.magazine.exception.MagazineExceptions; import lombok.AccessLevel; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.ToString; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; +import java.util.function.Supplier; import java.util.stream.Collectors; +/** + * Registry of the {@link Magazine} handles a client has opened. + */ @Getter @EqualsAndHashCode(of = "clientId") @ToString(of = "clientId") @@ -44,17 +51,92 @@ public MagazineManager(final String clientId) { } /** - * rebuilds magazineMap wrt the list of magazines provided + * Replaces the entire registry with the magazines provided. + *

+ * Every magazine not in {@code magazines} is evicted. Calling this with a single magazine + * therefore unregisters all the others, and the next access to one of them fails with + * {@link ErrorCode#MAGAZINE_NOT_FOUND} until it is rebuilt. Use {@link #register(Magazine)} to + * add one magazine. * - * @param magazines + * @param magazines the complete set of magazines this manager should serve. */ - public void refresh(final List> magazines) { + public void replaceAll(final List> magazines) { magazineMap.set(magazines.stream() .collect(Collectors.toUnmodifiableMap( Magazine::getMagazineIdentifier, Function.identity()))); } + /** + * Adds a magazine, leaving every other registration untouched, and replacing any existing + * registration under the same identifier. + * + * @param magazine the magazine to register. + */ + public void register(final Magazine magazine) { + if (Objects.isNull(magazine)) { + throw MagazineExceptions.invalidConfiguration("Magazine is required."); + } + magazineMap.updateAndGet(current -> copyWith(current, magazine)); + } + + /** + * Returns the magazine registered under {@code magazineIdentifier}, building and registering + * one through {@code factory} only if none exists. + * + * @param magazineIdentifier identifier to look up. + * @param factory builds the magazine when it is absent. Must not return null. + * @return the registered magazine. + */ + @SuppressWarnings("unchecked") + public Magazine getOrRegister(final String magazineIdentifier, + final Supplier> factory) { + if (Objects.isNull(magazineIdentifier) || magazineIdentifier.isBlank()) { + throw MagazineExceptions.invalidConfiguration("Magazine identifier is required."); + } + if (Objects.isNull(factory)) { + throw MagazineExceptions.invalidConfiguration("Magazine factory is required."); + } + final Optional> existing = find(magazineIdentifier); + if (existing.isPresent()) { + return existing.get(); + } + final Magazine built = factory.get(); + if (Objects.isNull(built)) { + throw MagazineExceptions.invalidConfiguration( + String.format("Magazine factory returned null for identifier %s.", magazineIdentifier)); + } + if (!magazineIdentifier.equals(built.getMagazineIdentifier())) { + throw MagazineExceptions.invalidConfiguration(String.format( + "Magazine factory for identifier %s returned a magazine identified as %s.", + magazineIdentifier, built.getMagazineIdentifier())); + } + // Only the first writer wins, so concurrent builders converge on one handle rather than + // each overwriting the last. + return (Magazine) magazineMap.updateAndGet(current -> current.containsKey(magazineIdentifier) + ? current + : copyWith(current, built)) + .get(magazineIdentifier); + } + + /** + * Removes a magazine from the registry. The underlying storage is untouched. + * + * @param magazineIdentifier identifier to drop. + * @return true if a registration was removed. + */ + public boolean unregister(final String magazineIdentifier) { + if (Objects.isNull(magazineIdentifier) || !magazineMap.get().containsKey(magazineIdentifier)) { + return false; + } + final Map> previous = magazineMap.getAndUpdate(current -> { + final Map> updated = new HashMap<>(current); + updated.remove(magazineIdentifier); + return Map.copyOf(updated); + }); + return previous.containsKey(magazineIdentifier); + } + public Map> getMagazineMap() { return magazineMap.get(); } @@ -66,15 +148,32 @@ public Map> getMagazineMap() { * @return magazine corresponding to provided magazineIdentifier * @param */ - @SuppressWarnings("unchecked") public Magazine getMagazine(final String magazineIdentifier) { - Magazine magazine = (Magazine) (magazineMap.get().get(magazineIdentifier)); - if (Objects.isNull(magazine)) { - throw MagazineException.builder() - .message(String.format("Magazine not found for identifier %s", magazineIdentifier)) - .errorCode(ErrorCode.MAGAZINE_NOT_FOUND) - .build(); + return this.find(magazineIdentifier) + .orElseThrow(() -> MagazineException.builder() + .message(String.format("Magazine not found for identifier %s", magazineIdentifier)) + .errorCode(ErrorCode.MAGAZINE_NOT_FOUND) + .build()); + } + + /** + * Looks up a magazine without treating absence as an error. + * + * @param magazineIdentifier identifier to look up. A null or blank identifier is simply absent. + * @return the registered magazine, or empty. + */ + @SuppressWarnings("unchecked") + public Optional> find(final String magazineIdentifier) { + if (Objects.isNull(magazineIdentifier) || magazineIdentifier.isBlank()) { + return Optional.empty(); } - return magazine; + return Optional.ofNullable((Magazine) magazineMap.get().get(magazineIdentifier)); + } + + private static Map> copyWith(final Map> current, + final Magazine magazine) { + final Map> updated = new HashMap<>(current); + updated.put(magazine.getMagazineIdentifier(), magazine); + return Map.copyOf(updated); } } diff --git a/magazine-core/src/test/java/com/phonepe/magazine/MagazineManagerTest.java b/magazine-core/src/test/java/com/phonepe/magazine/MagazineManagerTest.java new file mode 100644 index 0000000..c77c974 --- /dev/null +++ b/magazine-core/src/test/java/com/phonepe/magazine/MagazineManagerTest.java @@ -0,0 +1,271 @@ +/** + * Copyright (c) 2025 Original Author(s), PhonePe India Pvt. Ltd. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.phonepe.magazine; + +import com.phonepe.magazine.core.BaseMagazineStorage; +import com.phonepe.magazine.entity.FireCheckpoint; +import com.phonepe.magazine.entity.MagazineContext; +import com.phonepe.magazine.entity.MagazineData; +import com.phonepe.magazine.entity.MagazineScope; +import com.phonepe.magazine.entity.MetaData; +import com.phonepe.magazine.entity.StorageType; +import com.phonepe.magazine.exception.ErrorCode; +import com.phonepe.magazine.exception.MagazineException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.time.Instant; +import java.util.Collection; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class MagazineManagerTest { + + private MagazineManager manager; + private AtomicInteger initializations; + + @BeforeEach + void setup() { + manager = new MagazineManager("CLIENT_ID"); + initializations = new AtomicInteger(); + } + + @Test + @DisplayName("replaceAll evicts everything absent from the list") + void replaceAllEvicts() { + manager.replaceAll(List.of(magazine("A"), magazine("B"))); + manager.replaceAll(List.of(magazine("C"))); + + assertEquals(Set.of("C"), manager.getMagazineMap().keySet()); + assertThrows(MagazineException.class, () -> manager.getMagazine("A")); + } + + @Test + @DisplayName("register adds without disturbing anything already registered") + void registerIsAdditive() { + manager.replaceAll(List.of(magazine("A"), magazine("B"))); + manager.register(magazine("C")); + + assertEquals(Set.of("A", "B", "C"), manager.getMagazineMap().keySet()); + } + + @Test + @DisplayName("registering the same identifier twice swaps the handle rather than duplicating it") + void registerReplacesSameIdentifier() { + manager.register(magazine("A")); + final Magazine replacement = magazine("A"); + manager.register(replacement); + + assertEquals(1, manager.getMagazineMap().size()); + assertSame(replacement, manager.getMagazine("A")); + } + + @Test + @DisplayName("getOrRegister does not build a magazine that is already registered") + void getOrRegisterSkipsTheFactory() { + final Magazine first = manager.getOrRegister("A", () -> magazine("A")); + assertEquals(1, initializations.get(), "the first call must build"); + + final Magazine second = manager.getOrRegister("A", () -> magazine("A")); + assertSame(first, second); + assertEquals(1, initializations.get(), + "a registered magazine must not be rebuilt; building costs a shard-config read"); + } + + @Test + @DisplayName("getOrRegister rejects a factory that returns a differently identified magazine") + void getOrRegisterValidatesTheFactoryResult() { + assertEquals(ErrorCode.INVALID_CONFIGURATION, + assertThrows(MagazineException.class, + () -> manager.getOrRegister("A", () -> magazine("B"))).getErrorCode()); + assertTrue(manager.getMagazineMap().isEmpty(), "a rejected factory must register nothing"); + } + + @Test + @DisplayName("find reports absence without throwing, and getMagazine still throws") + void findDoesNotThrowOnAbsence() { + manager.register(magazine("A")); + + assertSame(manager.getMagazine("A"), manager.find("A").orElseThrow()); + assertTrue(manager.find("MISSING").isEmpty()); + assertTrue(manager.find(null).isEmpty(), "a null identifier is absent, not an error"); + assertTrue(manager.find(" ").isEmpty()); + + assertEquals(ErrorCode.MAGAZINE_NOT_FOUND, + assertThrows(MagazineException.class, () -> manager.getMagazine("MISSING")).getErrorCode()); + } + + @Test + @DisplayName("find lets a caller own the miss path without building on a hit") + void findSupportsCallerOwnedMissPath() { + for (int attempt = 0; attempt < 3; attempt++) { + manager.find("A").orElseGet(() -> { + final Magazine built = magazine("A"); + manager.register(built); + return built; + }); + } + assertEquals(1, initializations.get(), "only the first miss may build"); + } + + @Test + @DisplayName("unregister drops one magazine and reports whether it was there") + void unregisterDropsOne() { + manager.replaceAll(List.of(magazine("A"), magazine("B"))); + + assertTrue(manager.unregister("A")); + assertFalse(manager.unregister("A")); + assertEquals(Set.of("B"), manager.getMagazineMap().keySet()); + } + + @Test + @DisplayName("concurrent registrations all survive") + void concurrentRegistrationsDoNotClobberEachOther() throws Exception { + final int count = 32; + runConcurrently(count, index -> manager.register(magazine("M" + index))); + + assertEquals(count, manager.getMagazineMap().size(), + "a get-then-set registry would lose updates here"); + } + + @Test + @DisplayName("concurrent getOrRegister on one identifier hands every caller the same handle") + void concurrentGetOrRegisterConvergesOnOneHandle() throws Exception { + final int threads = 16; + final Magazine[] seen = new Magazine[threads]; + runConcurrently(threads, index -> seen[index] = manager.getOrRegister("A", () -> magazine("A"))); + + final Magazine registered = manager.getMagazine("A"); + for (int i = 0; i < threads; i++) { + assertSame(registered, seen[i], + "every caller must receive the registration that won, not its own losing build"); + } + } + + // --- helpers --------------------------------------------------------------------------- + + private interface IndexedAction { + void run(int index); + } + + private void runConcurrently(final int count, final IndexedAction action) throws Exception { + final ExecutorService pool = Executors.newFixedThreadPool(Math.min(count, 8)); + final CountDownLatch start = new CountDownLatch(1); + try { + final List> futures = new java.util.ArrayList<>(); + for (int i = 0; i < count; i++) { + final int index = i; + futures.add(pool.submit(() -> { + start.await(); + action.run(index); + return null; + })); + } + start.countDown(); + for (java.util.concurrent.Future future : futures) { + future.get(30, TimeUnit.SECONDS); + } + } finally { + pool.shutdownNow(); + } + } + + private Magazine magazine(final String identifier) { + return Magazine.builder() + .magazineIdentifier(identifier) + .baseMagazineStorage(new CountingStorage(initializations)) + .build(); + } + + /** + * Counts {@code initialize} calls, which is what a rebuild costs in the real backend. + */ + private static final class CountingStorage extends BaseMagazineStorage { + + private final AtomicInteger initializations; + + private CountingStorage(final AtomicInteger initializations) { + super(StorageType.AEROSPIKE, 60, 120, "FARM_ID", false, "CLIENT_ID", MagazineScope.LOCAL); + this.initializations = initializations; + } + + @Override + public MagazineContext initialize(final String magazineIdentifier) { + initializations.incrementAndGet(); + return new MagazineContext(magazineIdentifier, 2, 1); + } + + @Override + public boolean load(final MagazineContext context, final String data) { + return true; + } + + @Override + public boolean reload(final MagazineContext context, final String data) { + return true; + } + + @Override + public MagazineData fire(final MagazineContext context) { + return null; + } + + @Override + public Map getMetaData(final MagazineContext context) { + return Map.of(); + } + + @Override + public void delete(final MagazineContext context, final MagazineData magazineData) { + // no-op + } + + @Override + public void deleteAll(final MagazineContext context, final Collection> magazineData) { + // no-op + } + + @Override + public Set> peek(final MagazineContext context, + final Map> shardPointersMap) { + return Set.of(); + } + + @Override + public Map firePointerBefore(final MagazineContext context, final Instant instant) { + return Map.of(); + } + + @Override + public Map> fireHistory(final MagazineContext context) { + return Map.of(); + } + } +} diff --git a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineLifecycleTest.java b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineLifecycleTest.java index 1d164d5..7c864f4 100644 --- a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineLifecycleTest.java +++ b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineLifecycleTest.java @@ -46,7 +46,7 @@ class MagazineLifecycleTest extends AerospikeMagazineTestBase { @BeforeEach void setup() { magazineManager = new MagazineManager("CLIENT_ID"); - magazineManager.refresh(List.of(Magazine.builder() + magazineManager.replaceAll(List.of(Magazine.builder() .magazineIdentifier("MAGAZINE_ID1") .baseMagazineStorage(buildMagazineStorage(String.class)) .build(), diff --git a/magazine-coverage/pom.xml b/magazine-coverage/pom.xml index 207a064..749c42a 100644 --- a/magazine-coverage/pom.xml +++ b/magazine-coverage/pom.xml @@ -7,7 +7,7 @@ com.phonepe magazine - 2.0.0-SNAPSHOT + 2.0.1-SNAPSHOT magazine-coverage diff --git a/magazine-dw-bundle/pom.xml b/magazine-dw-bundle/pom.xml index cd50711..a304fbd 100644 --- a/magazine-dw-bundle/pom.xml +++ b/magazine-dw-bundle/pom.xml @@ -7,7 +7,7 @@ com.phonepe magazine - 2.0.0-SNAPSHOT + 2.0.1-SNAPSHOT magazine-dw-bundle diff --git a/magazine-dw-bundle/src/test/java/com/phonepe/magazine/demo/MagazineDashboardDemoApplication.java b/magazine-dw-bundle/src/test/java/com/phonepe/magazine/demo/MagazineDashboardDemoApplication.java index eab07df..58e263c 100644 --- a/magazine-dw-bundle/src/test/java/com/phonepe/magazine/demo/MagazineDashboardDemoApplication.java +++ b/magazine-dw-bundle/src/test/java/com/phonepe/magazine/demo/MagazineDashboardDemoApplication.java @@ -103,7 +103,7 @@ public void run(final MagazineDashboardDemoConfiguration configuration, final En new MagazineData<>("payment:PAY-202603", 47, 1, "payment-retries")), // Fire history left off, so the dashboard shows the disabled state rather than an error. null); - magazineBundle.getMagazineManager().refresh(java.util.List.of(emailJobs, paymentRetries)); + magazineBundle.getMagazineManager().replaceAll(java.util.List.of(emailJobs, paymentRetries)); } /** diff --git a/pom.xml b/pom.xml index c79341d..710b983 100644 --- a/pom.xml +++ b/pom.xml @@ -7,7 +7,7 @@ com.phonepe magazine - 2.0.0-SNAPSHOT + 2.0.1-SNAPSHOT pom Magazine From 44ee0d61a6d455cc4875b019220ad96dfd57bc1c Mon Sep 17 00:00:00 2001 From: Shantanu Tiwari Date: Thu, 15 Oct 2026 17:26:51 +0530 Subject: [PATCH 07/10] Cut fire-history write load and register meters lazily --- docs/docs/backends/aerospike.md | 78 +++- docs/docs/concepts/api-reference.md | 4 +- docs/docs/concepts/defaults.md | 2 +- docs/docs/concepts/metrics.md | 12 +- docs/docs/upgrading.md | 17 + docs/docs/usage.md | 11 +- .../impl/aerospike/AerospikeStorage.java | 3 +- .../aerospike/common/AerospikeConstants.java | 5 +- .../aerospike/common/AerospikePolicies.java | 7 + .../aerospike/store/FireHistoryStore.java | 393 ++++++++++++++++++ .../store/MagazineMetadataStore.java | 258 +++--------- .../magazine/metrics/MagazineMetrics.java | 103 +++-- .../magazine/metrics/StorageOperation.java | 2 +- .../aerospike/MagazineFireHistoryTest.java | 155 ++++++- .../impl/aerospike/MagazineMetricsTest.java | 24 ++ 15 files changed, 779 insertions(+), 295 deletions(-) create mode 100644 magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java diff --git a/docs/docs/backends/aerospike.md b/docs/docs/backends/aerospike.md index 0e5fdb6..8438e01 100644 --- a/docs/docs/backends/aerospike.md +++ b/docs/docs/backends/aerospike.md @@ -124,24 +124,74 @@ was actually read, so a crash leaves it short, which keeps the shard active and ## Fire history -When `fireHistoryEnabled` is set, each shard's **unified metadata record** carries an extra map bin -holding delivery-time checkpoints: window start to the fire pointer reached in that window. - -- **One entry per shard per window**, regardless of throughput. -- **Checkpoints are written during the active-shard refresh, not on the claim path.** That refresh - already batch-reads every shard's pointer record, so the numbers being recorded are already in - hand and no extra read is needed. Deliberately *not* coupled to the claim: claims are the hot path - and are far more frequent than the resolution a window needs. -- `firePointerBefore` also records before it resolves, so a magazine whose history cannot yet answer - still accumulates usable history rather than being stuck unable to ever answer. -- **Each shard's checkpoint is claimed independently**, and the claim is released if the write fails, - so a shard that errored is retried next round instead of sitting the window out. +When `fireHistoryEnabled` is set, each shard's **unified metadata record** carries an extra map bin, +`FIRE_HISTORY`, holding delivery-time checkpoints: `label -> fire pointer`, meaning "by `label`, the +fire pointer had reached at least this value". Keys are stored negated so the key-ordered map sorts +newest first. The logic lives in `FireHistoryStore`. + +### Windows and labels + +Time is cut into fixed windows of `fireHistoryWindowSeconds`, aligned to the epoch. Each shard gets +**at most one checkpoint per window**, regardless of throughput or how many processes serve the +magazine. + +A checkpoint is labelled with the **end** of the window it was observed in. The label must never be +earlier than the moment the pointer was read, or an answer would claim slots were taken before they +were; the end of the window is always at or after the observation. The consequence is that a checkpoint +becomes usable one window after it is taken. + +### When a checkpoint is written + +Checkpoints are written from the **active-shard refresh**, not the claim path. The refresh already +batch-reads every shard's pointer record; with fire history on, that same read also fetches each +shard's **newest checkpoint only** (one map entry, not the whole map). A shard is then skipped, with +no call at all, when: + +- **This window is already recorded** — its newest label is this window's. Another process, or an + earlier call here, wrote it. +- **The pointer has not moved** — its newest checkpoint already holds the current fire pointer. The + fire pointer only increases, so that older checkpoint stays true for every later instant. An idle + magazine costs nothing, however many processes serve it. + +Every remaining shard is written in **one batch call per magazine**, not one call per shard. + +### Spreading writes across processes + +Every consuming process runs the refresh. To stop them all writing right after each window boundary, +each process waits a **random offset into the first half of the window**, drawn once per magazine and +kept for the life of the process. The first process past its offset writes; the others read that +checkpoint on their next refresh and skip. This is always on with fire history and is not +configurable. + +### Writes from `firePointerBefore` + +`firePointerBefore` reads the whole history, then applies the same rules before resolving — never +delayed by the offset. This keeps history advancing when nothing is consuming (the refresh only runs +while something calls `fire()`), so claims made just before consumption stopped still become visible. +In steady state the refresh has already recorded the window and this writes nothing. + +### Safety and retention + +- **Best-effort.** A failed write leaves the previous checkpoint in place, which is still true. It + never fails the refresh or the read that triggered it. +- **Per-shard claims in each process** stop concurrent callers here from writing the same window, + and a claim is released if its write fails, so that shard is retried on the next refresh. +- **Create-only per label.** If two processes race on a label, the first wins and the other is a + server-side no-op. - **Eviction is by count** (`fireHistoryEntries`), applied on write — by count rather than age so an idle magazine keeps reach instead of having its history wiped by the first write after a pause. + The map rides the pointer record that every claim rewrites, so larger values make every claim + slightly heavier. - **Legacy split metadata does not support it.** Fire history requires the unified metadata record. -The cost is bytes on a record that is already read and written by a periodic refresh, not an extra -operation on the delivery path. +### Cost + +| Magazine state | Calls per window | +|---|---| +| Idle (pointer unchanged) | none | +| Active | usually one batch write for the whole magazine, from one process | + +Plus one map entry per shard on a refresh read that already happens. ## Metadata schema versions diff --git a/docs/docs/concepts/api-reference.md b/docs/docs/concepts/api-reference.md index f7b6aa7..33629da 100644 --- a/docs/docs/concepts/api-reference.md +++ b/docs/docs/concepts/api-reference.md @@ -37,8 +37,8 @@ Magazine magazine = Magazine.builder() | `getShards()` | `int` | This magazine's **persisted** shard count (from the resolved `MagazineContext`), not the shard count configured on the storage. | | `peek(Map> shardPointersMap)` | `Set>` | Read specific records without consuming. | | `getMagazineIdentifier()` | `String` | The identifier this magazine is bound to. | -| `firePointerBefore(Instant instant)` | `Map` | Where each shard's fire pointer stood at `instant`. Requires `fireHistoryEnabled`. A shard is **absent** when no checkpoint reaches back that far — treat that as "do nothing here", not "empty". Throws `NOT_ENABLED`, or `INVALID_REQUEST` when the retained history no longer spans `instant`. | -| `fireHistory()` | `Map>` | Every retained checkpoint per shard, newest first. Requires `fireHistoryEnabled`. | +| `firePointerBefore(Instant instant)` | `Map` | Where each shard's fire pointer stood at `instant`. Requires `fireHistoryEnabled`. A shard is **absent** when no checkpoint reaches back that far — treat that as "do nothing here", not "empty". Throws `NOT_ENABLED`, or `INVALID_REQUEST` when the retained history no longer spans `instant`. Also records the current pointer if this window has no checkpoint yet and the pointer moved — usually nothing. | +| `fireHistory()` | `Map>` | Every retained checkpoint per shard, newest first. Requires `fireHistoryEnabled`. Read-only: never records a checkpoint. | !!! warning "`fire()` is at-most-once" A record is claimed by a guarded atomic increment of the fire pointer. If the client times out after the server applied the claim, the pointer has advanced and that record will never be delivered. `fire()` throws `NOTHING_TO_FIRE` when the magazine is drained and `RETRIES_EXHAUSTED` when it gave up while skipping pointer holes — the latter does **not** mean the queue is empty. See [Delivery Semantics](delivery-semantics.md). diff --git a/docs/docs/concepts/defaults.md b/docs/docs/concepts/defaults.md index a0a7c8e..0bc1484 100644 --- a/docs/docs/concepts/defaults.md +++ b/docs/docs/concepts/defaults.md @@ -15,7 +15,7 @@ | `maxFireHoleSkips` | `512` | Bounds `fire()`; exhausting it raises `RETRIES_EXHAUSTED`. | | `metricsEnabled` | `true` | Publishes to Micrometer's global registry unless a `meterRegistry` is set on the builder. | | `fireHistoryEnabled` | `false` | Records delivery-time checkpoints, which `firePointerBefore` reads. Off by default: the checkpoint map rides the pointer record. | -| `fireHistoryWindowSeconds` | `300` | Seconds per checkpoint, and so the resolution of every `firePointerBefore` answer. | +| `fireHistoryWindowSeconds` | `300` | Seconds per checkpoint, and so the resolution of every `firePointerBefore` answer. Background writes are spread over the first half of each window; not configurable. | | `fireHistoryEntries` | `32` | Checkpoints retained per shard. Evicted by **count**, not age. | ### `metaDataTtl` must outlive `recordTtl` diff --git a/docs/docs/concepts/metrics.md b/docs/docs/concepts/metrics.md index 2e16be2..4d567ae 100644 --- a/docs/docs/concepts/metrics.md +++ b/docs/docs/concepts/metrics.md @@ -100,7 +100,9 @@ regardless of who attached it. ## What is published -All meters are tagged `magazine`. +All meters are tagged `magazine`. Each is registered the first time it records, so a magazine that +has never fired carries no fire meters, and an operation that never ran has no counter. An absent +meter reads as zero in rates and sums. | Meter | Type | Tags | What it tells you | |---|---|---|---| @@ -118,7 +120,11 @@ All meters are tagged `magazine`. `claim_fire_pointer`, `increment_load_pointer`, `increment_load_counter`, `increment_fire_counter`, `decrement_fire_counter`, `batch_read_metadata`, `refresh_active_shards`, `read_data`, `write_data`, `delete_data`, `batch_delete_data`, `batch_read_data`, `claim_dedupe_marker`, -`withdraw_dedupe_marker`. +`withdraw_dedupe_marker`, `write_fire_history`, `read_fire_history`. + +`write_fire_history` counts **batch calls**: at most one per magazine per window, and none for a +magazine whose fire pointers have not moved. `read_fire_history` is the read behind +`firePointerBefore` and `fireHistory`. `refresh_active_shards` is the batch read behind an active-shard cache refresh, split out from `batch_read_metadata` so refresh traffic is attributable without a meter of its own. A magazine on @@ -138,6 +144,8 @@ Divide `magazine.aerospike.calls` by the matching outcome counter. Healthy stead | Shard discovery | 1–2 batches / `activeShardRefreshSeconds` / magazine | `refresh_active_shards`; fans out to every shard | | `delete()` | **1** | `delete_data` | | `deleteAll()` | **1** | `batch_delete_data`, whatever the batch size. A one-record batch takes the single-key path and counts as `delete_data` | +| Fire-history recording | **≤ 1** batch / window / magazine | `write_fire_history`, only if some shard's fire pointer moved and the window is not yet recorded by any process. Rides the shard-discovery read; no extra read | +| `firePointerBefore()` | **1**, plus **≤ 1** | `read_fire_history`, plus a `write_fire_history` batch only when the read finds the window unrecorded | **Calls per `fire()` does not rise with consumer count.** The claim is a guarded atomic increment, so concurrent consumers get distinct pointers and no claim is ever lost. If you see the ratio climb diff --git a/docs/docs/upgrading.md b/docs/docs/upgrading.md index 2740a57..c294c73 100644 --- a/docs/docs/upgrading.md +++ b/docs/docs/upgrading.md @@ -125,6 +125,23 @@ classpath. Declare it directly if you depended on it transitively. - **`Magazine.deleteAll(Collection>)`** — retire a whole batch in one round trip. Optional for callers; **mandatory for anyone implementing `BaseMagazineStorage`**, see section 4. +## Upgrading within 2.0 + +### 2.0.2 + +No API changes. Behaviour that can show up on dashboards and alerts: + +- **Fire history writes far less.** Checkpoints are written in one batch per magazine, only for shards + whose fire pointer moved, and normally by one process per window. `write_fire_history` now counts + batch calls, so expect a large drop. Batch writes need Aerospike server 6.0 or later, which batch + deletes already required. +- **Checkpoints are labelled with the end of their window**, not the start, so a label is never + earlier than the observation. `firePointerBefore` therefore answers about one window later than + before. Entries written by 2.0.1 keep their window-start labels and remain readable; a mixed + deployment during a rolling upgrade is safe. +- **Meters are registered on first use.** An operation or outcome that never happened has no meter; + rates and sums read it as zero, but an alert that expects the series to exist needs a default. + ## Checklist 1. Change the artifact to `magazine-core`. diff --git a/docs/docs/usage.md b/docs/docs/usage.md index 0aa3030..e73b195 100644 --- a/docs/docs/usage.md +++ b/docs/docs/usage.md @@ -157,6 +157,10 @@ message. **Off by default.** The checkpoint map rides the shard's pointer record and every claim rewrites that record, so a caller that never asks should not pay for it. +Once on, checkpoints are recorded automatically while the magazine is consumed: at most one per shard +per window, none for a shard whose pointer has not moved, and one batch call per magazine. See +[Fire history](backends/aerospike.md#fire-history) for how. + ```java AerospikeStorageConfig.builder() .dataSetName("my_data") @@ -183,6 +187,9 @@ watermarks.forEach((shard, checkpoint) -> Every slot at or below the returned pointer was claimed at or before that instant. Slots above it might have been claimed a moment ago. +A checkpoint is labelled with the end of the window it was observed in, so it answers questions about +instants from one window later onwards. Precision is therefore one to two windows. + !!! note "An absent shard means 'do nothing', not 'empty'" A shard is missing from the result when no checkpoint reaches back that far yet. That is normal for a young magazine. Treat it as "nothing is provably old enough here this round" — never as @@ -192,8 +199,8 @@ might have been claimed a moment ago. | Setting | Meaning | |---|---| -| `fireHistoryWindowSeconds` | Seconds covered by one checkpoint, and therefore the **resolution** of every answer. One entry per shard per window, whatever the throughput | -| `fireHistoryEntries` | Checkpoints retained per shard. Eviction is by **count, not age**, so an intermittently used magazine reaches further back than `window x entries` rather than having its history wiped by the first write after an idle period | +| `fireHistoryWindowSeconds` | Seconds covered by one checkpoint, and therefore the **resolution** of every answer. At most one entry per shard per window, whatever the throughput, and none while the shard's pointer does not move. A wider window means fewer writes | +| `fireHistoryEntries` | Checkpoints retained per shard. Eviction is by **count, not age**, so an intermittently used magazine reaches further back than `window x entries` rather than having its history wiped by the first write after an idle period. The map rides the pointer record every claim rewrites, so raise this only as far as you need | Asking about an instant further back than the retained history throws `INVALID_REQUEST`, and asking at all with the feature disabled throws `NOT_ENABLED`. diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java index 9e444df..31d490b 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/AerospikeStorage.java @@ -34,6 +34,7 @@ import com.phonepe.magazine.impl.aerospike.common.ErrorMessage; import com.phonepe.magazine.impl.aerospike.store.ActiveShardSelector; import com.phonepe.magazine.impl.aerospike.store.DeDupeGuard; +import com.phonepe.magazine.impl.aerospike.store.FireHistoryStore; import com.phonepe.magazine.impl.aerospike.store.MagazineDataStore; import com.phonepe.magazine.impl.aerospike.store.MagazineMetadataStore; import com.phonepe.magazine.metrics.MagazineMetrics; @@ -106,7 +107,7 @@ public AerospikeStorage( namespace, metaSetName, storageConfig.getShards(), storageConfig.isAllowShardIncrease()); this.metadataStore = new MagazineMetadataStore(aerospikeClient, retryerFactory, metrics, namespace, metaSetName, getMetaDataTtl(), - new MagazineMetadataStore.FireHistoryConfig(storageConfig.isFireHistoryEnabled(), + new FireHistoryStore.FireHistoryConfig(storageConfig.isFireHistoryEnabled(), storageConfig.fireHistoryWindowMillis(), storageConfig.getFireHistoryEntries())); this.dataStore = new MagazineDataStore<>(aerospikeClient, retryerFactory, metrics, namespace, dataSetName, getRecordTtl(), clazz); diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikeConstants.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikeConstants.java index b195abe..7091013 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikeConstants.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikeConstants.java @@ -42,9 +42,8 @@ public final class AerospikeConstants { public static final String CREATED_AT = "CREATED_AT"; public static final String SHARDS_BIN = "SHARDS"; /** - * Delivery-time checkpoints: a key-ordered map of {@code negated window start -> fire pointer}. - * Keys are negated so the map sorts newest-first, which makes eviction a single index-range - * removal that keeps the newest N entries. + * Delivery-time checkpoints: key-ordered map of {@code -label -> fire pointer}, label being the + * observation window's end. Negated keys sort newest-first, so eviction is one index range. */ public static final String FIRE_HISTORY = "FIRE_HISTORY"; diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikePolicies.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikePolicies.java index 40c460a..97b7a6e 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikePolicies.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/common/AerospikePolicies.java @@ -19,6 +19,7 @@ import com.aerospike.client.IAerospikeClient; import com.aerospike.client.policy.BatchDeletePolicy; import com.aerospike.client.policy.BatchPolicy; +import com.aerospike.client.policy.BatchWritePolicy; import com.aerospike.client.policy.WritePolicy; import java.util.Objects; import lombok.AccessLevel; @@ -52,4 +53,10 @@ public static BatchDeletePolicy batchDeletePolicy(final IAerospikeClient client) final BatchDeletePolicy defaults = client.getBatchDeletePolicyDefault(); return Objects.isNull(defaults) ? new BatchDeletePolicy() : new BatchDeletePolicy(defaults); } + + /** @return a copy of the client's default batch write policy, or a fresh default. */ + public static BatchWritePolicy batchWritePolicy(final IAerospikeClient client) { + final BatchWritePolicy defaults = client.getBatchWritePolicyDefault(); + return Objects.isNull(defaults) ? new BatchWritePolicy() : new BatchWritePolicy(defaults); + } } diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java new file mode 100644 index 0000000..dad52d7 --- /dev/null +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java @@ -0,0 +1,393 @@ +/** + * Copyright (c) 2025 Original Author(s), PhonePe India Pvt. Ltd. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.phonepe.magazine.impl.aerospike.store; + +import com.aerospike.client.BatchRecord; +import com.aerospike.client.BatchWrite; +import com.aerospike.client.IAerospikeClient; +import com.aerospike.client.Key; +import com.aerospike.client.Operation; +import com.aerospike.client.Record; +import com.aerospike.client.ResultCode; +import com.aerospike.client.Value; +import com.aerospike.client.cdt.MapOperation; +import com.aerospike.client.cdt.MapOrder; +import com.aerospike.client.cdt.MapPolicy; +import com.aerospike.client.cdt.MapReturnType; +import com.aerospike.client.cdt.MapWriteFlags; +import com.aerospike.client.policy.BatchPolicy; +import com.aerospike.client.policy.BatchWritePolicy; +import com.aerospike.client.policy.RecordExistsAction; +import com.phonepe.magazine.entity.FireCheckpoint; +import com.phonepe.magazine.entity.MagazineContext; +import com.phonepe.magazine.exception.ErrorCode; +import com.phonepe.magazine.exception.MagazineExceptions; +import com.phonepe.magazine.impl.aerospike.common.AerospikeConstants; +import com.phonepe.magazine.impl.aerospike.common.AerospikePolicies; +import com.phonepe.magazine.impl.aerospike.common.AerospikeRetryer; +import com.phonepe.magazine.impl.aerospike.common.ErrorMessage; +import com.phonepe.magazine.metrics.MagazineMetrics; +import com.phonepe.magazine.metrics.StorageOperation; +import java.time.Instant; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.atomic.AtomicLong; +import java.util.function.Function; +import java.util.stream.Stream; +import lombok.Getter; +import lombok.extern.slf4j.Slf4j; + +/** + * Delivery-time checkpoints: per-shard map of {@code -label -> fire pointer} (newest first) on the + * pointer record. Recorded from the active-shard refresh and from {@code firePointerBefore}; at most + * one per shard per window, labelled with the window end (never earlier than the observation). + * Skipped when the window is already recorded or the pointer is unchanged. One batch write per + * magazine; refresh writes wait for a random per-process offset in the first half of the window. + * Best-effort: failures never fail the triggering call. + */ +@Slf4j +public final class FireHistoryStore { + + /** A second writer for the same label is a server-side no-op. */ + private static final MapPolicy CHECKPOINT_MAP_POLICY = new MapPolicy( + MapOrder.KEY_ORDERED, MapWriteFlags.CREATE_ONLY | MapWriteFlags.NO_FAIL); + + private static final Operation READ_NEWEST_CHECKPOINT = MapOperation.getByIndex( + AerospikeConstants.FIRE_HISTORY, 0, MapReturnType.KEY_VALUE); + + private final IAerospikeClient client; + private final AerospikeRetryer retryer; + private final MagazineMetrics metrics; + private final Function> pointerKeys; + + private final boolean enabled; + private final long windowMillis; + private final int entries; + + private final BatchPolicy batchPolicy; + private final BatchWritePolicy writePolicy; + + /** Bounded by the number of configured magazines. */ + private final Map states = new ConcurrentHashMap<>(); + + /** + * @param windowMillis recording window width; the resolution of every answer. + * @param entries checkpoints retained per shard, evicted by count. + */ + public record FireHistoryConfig(boolean enabled, long windowMillis, int entries) { + } + + public FireHistoryStore(final IAerospikeClient client, + final AerospikeRetryer retryer, + final MagazineMetrics metrics, + final Function> pointerKeys, + final int metaDataTtl, + final FireHistoryConfig config) { + this.client = client; + this.retryer = retryer; + this.metrics = metrics; + this.pointerKeys = pointerKeys; + this.enabled = config.enabled(); + this.windowMillis = config.windowMillis(); + this.entries = config.entries(); + + this.batchPolicy = AerospikePolicies.batchPolicy(client); + // UPDATE_ONLY: never create a pointer record for an uninitialised shard. + this.writePolicy = AerospikePolicies.batchWritePolicy(client); + this.writePolicy.recordExistsAction = RecordExistsAction.UPDATE_ONLY; + this.writePolicy.expiration = metaDataTtl; + } + + /** Extra refresh-read operation: only the newest checkpoint, as {@link #recordOnRefresh} needs. */ + public Optional refreshReadOperation() { + return enabled ? Optional.of(READ_NEWEST_CHECKPOINT) : Optional.empty(); + } + + /** Records from the refresh read (must include the newest checkpoint), after this process's offset. */ + public void recordOnRefresh(final MagazineContext context, final Record[] pointerRecords) { + if (!enabled) { + return; + } + final long now = System.currentTimeMillis(); + final Window window = Window.containing(now, windowMillis); + if (now >= window.start() + state(context).getJitterMillis()) { + record(context, pointerRecords, window); + } + } + + /** + * Lower bound of each shard's fire pointer at {@code instant}; may lag, never leads. + * + * @return shard id to checkpoint; absent when no checkpoint is old enough yet. + * @throws com.phonepe.magazine.exception.MagazineException {@code NOT_ENABLED} if disabled, + * {@code INVALID_REQUEST} if retained history does not reach {@code instant}. + */ + public Map firePointerBefore(final MagazineContext context, final Instant instant) { + requireEnabled(context); + final Record[] records = readHistory(context); + + // Record first (undelayed) so history still advances even if resolving throws. + record(context, records, Window.containing(System.currentTimeMillis(), windowMillis)); + + final Map answers = new HashMap<>(capacityFor(context.getShards())); + for (int shard = 0; shard < context.getShards(); shard++) { + final int current = shard; + resolve(context, current, checkpoints(records[current]), instant) + .ifPresent(checkpoint -> answers.put(context.shardId(current), checkpoint.toEntity())); + } + return answers; + } + + /** Every retained checkpoint per shard, newest first. Reading never records. */ + public Map> history(final MagazineContext context) { + requireEnabled(context); + final Record[] records = readHistory(context); + final Map> history = new HashMap<>(capacityFor(context.getShards())); + for (int shard = 0; shard < context.getShards(); shard++) { + final List checkpoints = checkpoints(records[shard]).stream() + .map(Checkpoint::toEntity) + .toList(); + if (!checkpoints.isEmpty()) { + history.put(context.shardId(shard), checkpoints); + } + } + return history; + } + + // --- recording --------------------------------------------------------------------------- + + private void record(final MagazineContext context, final Record[] pointerRecords, final Window window) { + final MagazineState state = state(context); + final List shards = shardsNeedingCheckpoint(context, pointerRecords, window, state); + if (!shards.isEmpty()) { + write(context, pointerRecords, shards, window, state); + } + } + + /** Shards that need this window's checkpoint, each already claimed for this process. */ + private static List shardsNeedingCheckpoint(final MagazineContext context, + final Record[] pointerRecords, + final Window window, + final MagazineState state) { + final List shards = new ArrayList<>(); + for (int shard = 0; shard < context.getShards(); shard++) { + final Record pointers = pointerRecords[shard]; + if (Objects.nonNull(pointers) + && needsCheckpoint(pointers, window) + && state.claim(shard, window)) { + shards.add(shard); + } + } + return shards; + } + + private static boolean needsCheckpoint(final Record pointers, final Window window) { + final Optional newest = checkpoints(pointers).stream().findFirst(); + if (newest.isEmpty()) { + return true; + } + final boolean windowAlreadyRecorded = newest.get().label() >= window.label(); + // Fire pointer is monotonic: an unchanged value stays true, so skip idle shards. + final boolean pointerUnchanged = newest.get().firePointer() == firePointer(pointers); + return !windowAlreadyRecorded && !pointerUnchanged; + } + + /** One batch call for every shard; a failed shard's claim is released so it is retried. */ + private void write(final MagazineContext context, + final Record[] pointerRecords, + final List shards, + final Window window, + final MagazineState state) { + final List keys = pointerKeys.apply(context); + final List writes = new ArrayList<>(shards.size()); + for (int shard : shards) { + writes.add(checkpointWrite(keys.get(shard), window.label(), firePointer(pointerRecords[shard]))); + } + + try { + metrics.aerospikeCall(context.getMagazineIdentifier(), StorageOperation.WRITE_FIRE_HISTORY); + client.operate(batchPolicy, writes); + } catch (Exception e) { + shards.forEach(shard -> state.release(shard, window)); + log.warn("Could not record fire history for magazine {} window {}.", + context.getMagazineIdentifier(), window.start(), e); + return; + } + + for (int i = 0; i < writes.size(); i++) { + final int resultCode = writes.get(i).resultCode; + if (resultCode == ResultCode.OK) { + continue; + } + final int shard = shards.get(i); + state.release(shard, window); + // A missing record is expected: the shard has none yet, or it expired. + if (resultCode != ResultCode.KEY_NOT_FOUND_ERROR) { + log.warn("Could not record fire history for magazine {} shard {} window {}: {}.", + context.getMagazineIdentifier(), shard, window.start(), + ResultCode.getResultString(resultCode)); + } + } + } + + private BatchWrite checkpointWrite(final Key key, final long label, final long firePointer) { + return new BatchWrite(writePolicy, key, new Operation[]{ + MapOperation.put(CHECKPOINT_MAP_POLICY, AerospikeConstants.FIRE_HISTORY, + Value.get(Checkpoint.encode(label)), Value.get(firePointer)), + // Evict by count, not age, so a long idle period does not wipe usable history. + MapOperation.removeByIndexRange(AerospikeConstants.FIRE_HISTORY, entries, MapReturnType.NONE)}); + } + + private MagazineState state(final MagazineContext context) { + return states.computeIfAbsent(context, this::newState); + } + + private MagazineState newState(final MagazineContext context) { + // Random offset in the first half of the window, fixed per magazine per process. + final long span = windowMillis / 2; + final long jitter = span <= 0 ? 0L : ThreadLocalRandom.current().nextLong(span); + return new MagazineState(context.getShards(), jitter); + } + + // --- reading ----------------------------------------------------------------------------- + + private Record[] readHistory(final MagazineContext context) { + return retryer.call(() -> { + metrics.aerospikeCall(context.getMagazineIdentifier(), StorageOperation.READ_FIRE_HISTORY); + return client.get(client.getBatchPolicyDefault(), + pointerKeys.apply(context).toArray(new Key[0]), + AerospikeConstants.getFireHistoryBins()); + }); + } + + /** The newest checkpoint labelled at or before {@code instant}. */ + private Optional resolve(final MagazineContext context, + final int shard, + final List checkpoints, + final Instant instant) { + final long cutoff = instant.toEpochMilli(); + final Optional answer = checkpoints.stream() + .filter(checkpoint -> checkpoint.label() <= cutoff) + .findFirst(); + // At capacity with nothing old enough: the answer was evicted; fail rather than return empty. + if (answer.isEmpty() && checkpoints.size() >= entries) { + throw MagazineExceptions.invalidRequest(String.format(ErrorMessage.FIRE_HISTORY_TOO_SHORT, + context.getMagazineIdentifier(), shard, instant, + Instant.ofEpochMilli(checkpoints.get(checkpoints.size() - 1).label()))); + } + return answer; + } + + /** Checkpoints newest first, from either the full map or the refresh read's single entry. */ + private static List checkpoints(final Record pointerRecord) { + final Object raw = Objects.isNull(pointerRecord) ? null : pointerRecord.getValue(AerospikeConstants.FIRE_HISTORY); + final Stream rawEntries; + if (raw instanceof Map map) { + rawEntries = map.entrySet().stream(); + } else if (raw instanceof List list) { + rawEntries = list.stream(); + } else { + rawEntries = Stream.ofNullable(raw); + } + return rawEntries.map(Checkpoint::decode) + .flatMap(Optional::stream) + .sorted(Checkpoint.NEWEST_FIRST) + .toList(); + } + + private void requireEnabled(final MagazineContext context) { + if (!enabled) { + throw MagazineExceptions.of(ErrorCode.NOT_ENABLED, String.format( + ErrorMessage.FIRE_HISTORY_DISABLED, context.getMagazineIdentifier())); + } + } + + private static long firePointer(final Record pointers) { + return pointers.getLong(AerospikeConstants.FIRE_POINTER); + } + + private static int capacityFor(final int expectedSize) { + return (int) Math.ceil(expectedSize / 0.75d) + 1; + } + + // --- value types ------------------------------------------------------------------------- + + /** A recording window; {@code label} is its end. */ + private record Window(long start, long label) { + + static Window containing(final long now, final long width) { + final long start = now / width * width; + return new Window(start, start + width); + } + } + + /** By {@code label}, the fire pointer had reached at least {@code firePointer}. */ + private record Checkpoint(long label, long firePointer) { + + static final Comparator NEWEST_FIRST = Comparator.comparingLong(Checkpoint::label).reversed(); + + /** Map keys are negated labels, so the key-ordered map sorts newest-first. */ + static long encode(final long label) { + return -label; + } + + static Optional decode(final Object raw) { + if (raw instanceof Map.Entry entry + && entry.getKey() instanceof Number key + && entry.getValue() instanceof Number value) { + return Optional.of(new Checkpoint(-key.longValue(), value.longValue())); + } + return Optional.empty(); + } + + FireCheckpoint toEntity() { + return new FireCheckpoint(firePointer, Instant.ofEpochMilli(label)); + } + } + + /** Per-process jitter and last claimed window per shard (released on failure for retry). */ + private static final class MagazineState { + private final AtomicLong[] claimedWindows; + @Getter + private final long jitterMillis; + + MagazineState(final int shards, final long jitterMillis) { + this.claimedWindows = new AtomicLong[shards]; + for (int shard = 0; shard < shards; shard++) { + claimedWindows[shard] = new AtomicLong(Long.MIN_VALUE); + } + this.jitterMillis = jitterMillis; + } + + boolean claim(final int shard, final Window window) { + final AtomicLong claimed = claimedWindows[shard]; + final long previous = claimed.get(); + return previous < window.start() && claimed.compareAndSet(previous, window.start()); + } + + void release(final int shard, final Window window) { + claimedWindows[shard].compareAndSet(window.start(), Long.MIN_VALUE); + } + } +} diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java index 3d63943..ec3a873 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java @@ -23,19 +23,13 @@ import com.aerospike.client.Operation; import com.aerospike.client.Record; import com.aerospike.client.ResultCode; -import com.aerospike.client.Value; -import com.aerospike.client.cdt.MapOperation; -import com.aerospike.client.cdt.MapOrder; -import com.aerospike.client.cdt.MapPolicy; -import com.aerospike.client.cdt.MapReturnType; -import com.aerospike.client.cdt.MapWriteFlags; import com.aerospike.client.exp.Exp; +import com.aerospike.client.exp.Expression; import com.aerospike.client.policy.RecordExistsAction; import com.aerospike.client.policy.WritePolicy; import com.phonepe.magazine.entity.FireCheckpoint; import com.phonepe.magazine.entity.MagazineContext; import com.phonepe.magazine.entity.MetaData; -import com.phonepe.magazine.exception.ErrorCode; import com.phonepe.magazine.exception.MagazineExceptions; import com.phonepe.magazine.impl.aerospike.common.AerospikeConstants; import com.phonepe.magazine.impl.aerospike.common.AerospikeNaming; @@ -45,18 +39,14 @@ import com.phonepe.magazine.metrics.MagazineMetrics; import com.phonepe.magazine.metrics.StorageOperation; import java.time.Instant; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Optional; import java.util.OptionalLong; -import java.util.SortedMap; -import java.util.TreeMap; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.atomic.AtomicLong; -import lombok.extern.slf4j.Slf4j; /** * Reads and writes the per-shard metadata records: load/fire pointers and load/fire counters. @@ -64,27 +54,21 @@ * Pointers and counters live in one record under the unified schema and in two under the legacy * schema; that difference is confined to {@link AerospikeNaming}, so every method here is written * once and works for both. - * + *

+ * Delivery-time checkpoints on the pointer record are owned by {@link FireHistoryStore}. + *

* The filter expression, the write policies and the metadata keys are all constant per magazine * and all resolved once: {@link Exp#build} serialises to a byte array, a policy is a ~25-field * copy, and {@link Key} construction runs a RIPEMD-160 digest. Rebuilding those per operation was * pure overhead on every round trip. */ -@Slf4j public final class MagazineMetadataStore { - /** - * {@code CREATE_ONLY | NO_FAIL} makes a second writer in the same window a server-side no-op - * rather than an error, which is what lets every process attempt the write independently. - */ - private static final MapPolicy CHECKPOINT_MAP_POLICY = new MapPolicy( - MapOrder.KEY_ORDERED, MapWriteFlags.CREATE_ONLY | MapWriteFlags.NO_FAIL); - /** * The claim's precondition: this shard still has an unconsumed slot. Absent bins read as zero, * so a record never fired from does not make the comparison unknown. */ - private static final com.aerospike.client.exp.Expression FIRE_POINTER_BEHIND_LOAD_POINTER = + private static final Expression FIRE_POINTER_BEHIND_LOAD_POINTER = Exp.build(Exp.lt( binOrZero(AerospikeConstants.FIRE_POINTER), binOrZero(AerospikeConstants.LOAD_POINTER))); @@ -97,26 +81,11 @@ public final class MagazineMetadataStore { private final WritePolicy claimPolicy; private final WritePolicy adjustPolicy; - private final WritePolicy checkpointPolicy; - private final boolean fireHistoryEnabled; - private final long fireHistoryWindowMillis; - private final int fireHistoryEntries; + /** Metadata bins, plus the newest checkpoint when fire history is on. */ + private final Operation[] refreshOperations; + private final FireHistoryStore fireHistory; /** Bounded by the number of configured magazines. */ private final Map keyCache = new ConcurrentHashMap<>(); - /** - * Last checkpoint window written, per shard, so only one caller per window issues each write. - * Per shard rather than per magazine: a shard skipped because its record did not exist yet, or - * because its write failed, must be retried within the same window rather than sit out until - * the next one. - *

- * Keyed by context like {@link #keyCache}, because one store serves every magazine on a - * storage. Shard count is part of the context's equality, so the array can never outlive the - * shard count it was sized for. - */ - private final Map checkpointWindows = new ConcurrentHashMap<>(); - - public record FireHistoryConfig(boolean enabled, long windowMillis, int entries) { - } public MagazineMetadataStore(final IAerospikeClient client, final AerospikeRetryer retryerFactory, @@ -124,15 +93,15 @@ public MagazineMetadataStore(final IAerospikeClient client, final String namespace, final String metaSetName, final int metaDataTtl, - final FireHistoryConfig fireHistoryConfig) { + final FireHistoryStore.FireHistoryConfig fireHistoryConfig) { this.client = client; this.retryerFactory = retryerFactory; this.metrics = metrics; this.namespace = namespace; this.metaSetName = metaSetName; - this.fireHistoryEnabled = fireHistoryConfig.enabled(); - this.fireHistoryWindowMillis = fireHistoryConfig.windowMillis(); - this.fireHistoryEntries = fireHistoryConfig.entries(); + this.fireHistory = new FireHistoryStore(client, retryerFactory, metrics, + context -> keys(context).pointerKeys(), metaDataTtl, fireHistoryConfig); + this.refreshOperations = refreshOperations(fireHistory); this.claimPolicy = AerospikePolicies.writePolicy(client); this.claimPolicy.recordExistsAction = RecordExistsAction.UPDATE_ONLY; @@ -144,12 +113,6 @@ public MagazineMetadataStore(final IAerospikeClient client, this.adjustPolicy = AerospikePolicies.writePolicy(client); this.adjustPolicy.recordExistsAction = RecordExistsAction.UPDATE; this.adjustPolicy.expiration = metaDataTtl; - - // UPDATE_ONLY: a checkpoint describes a shard that exists. Creating the record here would - // fabricate a pointer record for a shard that was never initialised. - this.checkpointPolicy = AerospikePolicies.writePolicy(client); - this.checkpointPolicy.recordExistsAction = RecordExistsAction.UPDATE_ONLY; - this.checkpointPolicy.expiration = metaDataTtl; } /** @@ -259,185 +222,32 @@ public Map readAll(final MagazineContext context) { * @return shard numbers with data available to fire, ascending. */ public int[] activeShards(final MagazineContext context) { - final Record[] pointerRecords = batchRead(context, keys(context).pointerKeys(), - StorageOperation.REFRESH_ACTIVE_SHARDS); + final Record[] pointerRecords = refreshRead(context); final Record[] counterRecords = AerospikeNaming.usesUnifiedMetadata(context) ? pointerRecords : batchRead(context, keys(context).counterKeys(), StorageOperation.REFRESH_ACTIVE_SHARDS); - // The batch read above already holds every shard's fire pointer, so recording checkpoints - // here costs no additional read, and this cache refreshes asynchronously. - recordFireHistory(context, pointerRecords); + // Reuses the read above; no extra call. + fireHistory.recordOnRefresh(context, pointerRecords); final int[] active = new int[context.getShards()]; int found = 0; for (int shard = 0; shard < context.getShards(); shard++) { - final Record pointers = pointerRecords[shard]; - final Record counters = counterRecords[shard]; - if (Objects.nonNull(pointers) - && Objects.nonNull(counters) - && counters.getLong(AerospikeConstants.LOAD_COUNTER) > counters.getLong(AerospikeConstants.FIRE_COUNTER) - && pointers.getLong(AerospikeConstants.LOAD_POINTER) > pointers.getLong(AerospikeConstants.FIRE_POINTER)) { + if (isActive(pointerRecords[shard], counterRecords[shard])) { active[found++] = shard; } } return found == active.length ? active : Arrays.copyOf(active, found); } - /** - * Where each shard's fire pointer stood at {@code instant}. - *

- * Answers come from checkpoints recorded at or before {@code instant}, so a returned pointer is - * a lower bound: every slot at or below it was claimed by then. It may lag reality, never lead - * it. - * - * @return shard id to checkpoint. A shard is absent when no checkpoint is old enough yet. - * @throws com.phonepe.magazine.exception.MagazineException {@code NOT_ENABLED} when fire - * history is disabled, {@code INVALID_REQUEST} when retained history does not reach - * back to {@code instant}. - */ + /** @see FireHistoryStore#firePointerBefore */ public Map firePointerBefore(final MagazineContext context, final Instant instant) { - if (!fireHistoryEnabled) { - throw MagazineExceptions.of(ErrorCode.NOT_ENABLED, String.format( - ErrorMessage.FIRE_HISTORY_DISABLED, context.getMagazineIdentifier())); - } - final Record[] records = readFireHistory(context); - - // Recorded before resolving, so a shard whose history cannot answer still advances: the - // throw below must not stop this magazine from ever accumulating usable history again. - recordFireHistory(context, records); - - final Map checkpoints = new HashMap<>(capacityFor(context.getShards())); - for (int shard = 0; shard < context.getShards(); shard++) { - final int current = shard; - resolveCheckpoint(context, records[current], instant, current) - .ifPresent(checkpoint -> checkpoints.put(context.shardId(current), checkpoint)); - } - return checkpoints; + return fireHistory.firePointerBefore(context, instant); } - /** - * Every retained checkpoint, newest first - */ + /** @see FireHistoryStore#history */ public Map> fireHistory(final MagazineContext context) { - if (!fireHistoryEnabled) { - throw MagazineExceptions.of(ErrorCode.NOT_ENABLED, String.format( - ErrorMessage.FIRE_HISTORY_DISABLED, context.getMagazineIdentifier())); - } - final Record[] records = readFireHistory(context); - final Map> history = new HashMap<>(capacityFor(context.getShards())); - for (int shard = 0; shard < context.getShards(); shard++) { - final List checkpoints = readHistory(records[shard]).entrySet().stream() - .map(entry -> new FireCheckpoint(entry.getValue(), Instant.ofEpochMilli(-entry.getKey()))) - .toList(); - if (!checkpoints.isEmpty()) { - history.put(context.shardId(shard), checkpoints); - } - } - return history; - } - - private Record[] readFireHistory(final MagazineContext context) { - return retryerFactory.call(() -> { - metrics.aerospikeCall(context.getMagazineIdentifier(), StorageOperation.READ_FIRE_HISTORY); - return client.get(client.getBatchPolicyDefault(), - keys(context).pointerKeys().toArray(new Key[0]), - AerospikeConstants.getFireHistoryBins()); - }); - } - - private Optional resolveCheckpoint(final MagazineContext context, - final Record pointerRecord, - final Instant instant, - final int shard) { - final SortedMap history = readHistory(pointerRecord); - final long cutoff = instant.toEpochMilli(); - for (Map.Entry entry : history.entrySet()) { - final long windowStart = -entry.getKey(); - if (windowStart <= cutoff) { - return Optional.of(new FireCheckpoint(entry.getValue(), Instant.ofEpochMilli(windowStart))); - } - } - // At capacity with nothing old enough means history that could have answered was evicted. - // Returning empty would be indistinguishable from "nothing to do" and would silently stop - // the caller from ever acting. - if (history.size() >= fireHistoryEntries) { - throw MagazineExceptions.invalidRequest(String.format(ErrorMessage.FIRE_HISTORY_TOO_SHORT, - context.getMagazineIdentifier(), shard, instant, - Instant.ofEpochMilli(-history.lastKey()))); - } - return Optional.empty(); - } - - @SuppressWarnings("unchecked") - private SortedMap readHistory(final Record pointerRecord) { - final Object raw = Objects.isNull(pointerRecord) ? null : pointerRecord.getValue(AerospikeConstants.FIRE_HISTORY); - return raw instanceof Map ? new TreeMap<>((Map) raw) : new TreeMap<>(); - } - - /** - * Records one checkpoint per shard, at most once per window per process. - *

- * Deliberately best-effort: a checkpoint improves the reader's precision but is never a - * correctness requirement, since a missed window leaves the previous checkpoint in place and - * that still states something true. Failing a cache refresh or a read because a checkpoint - * could not be written would turn a cosmetic loss into an outage. - *

- * Claimed per shard, and the claim is released again if the write fails, so a shard that had no - * record yet or whose write errored is retried on the next call rather than sitting the window - * out. - */ - private void recordFireHistory(final MagazineContext context, final Record[] pointerRecords) { - if (!fireHistoryEnabled) { - return; - } - final long window = System.currentTimeMillis() / fireHistoryWindowMillis * fireHistoryWindowMillis; - final AtomicLong[] claims = checkpointWindows.computeIfAbsent(context, this::newClaims); - final List pointerKeys = keys(context).pointerKeys(); - // Negated so the key-ordered map sorts newest-first and eviction is a single index range. - final Value windowKey = Value.get(-window); - - for (int shard = 0; shard < context.getShards(); shard++) { - final Record pointers = pointerRecords[shard]; - if (Objects.isNull(pointers) || !claim(claims[shard], window)) { - continue; - } - try { - writeCheckpoint(context, pointerKeys.get(shard), windowKey, - pointers.getLong(AerospikeConstants.FIRE_POINTER)); - } catch (Exception e) { - claims[shard].compareAndSet(window, Long.MIN_VALUE); - log.warn("Could not record fire history for magazine {} shard {} window {}.", - context.getMagazineIdentifier(), shard, window, e); - } - } - } - - private void writeCheckpoint(final MagazineContext context, final Key key, final Value windowKey, - final long firePointer) { - metrics.aerospikeCall(context.getMagazineIdentifier(), StorageOperation.WRITE_FIRE_HISTORY); - client.operate(checkpointPolicy, key, - // CREATE_ONLY|NO_FAIL: the first writer in a window wins and every other process is - // a server-side no-op. Every writer's value is a lower bound, so whichever wins - // recorded the most conservative pointer observed for that window. - MapOperation.put(CHECKPOINT_MAP_POLICY, AerospikeConstants.FIRE_HISTORY, - windowKey, Value.get(firePointer)), - // Keep the newest N by count, not by age: an age cutoff computed from the clock - // would discard the whole map on the first write after an idle period longer than - // the span, blinding readers even though those checkpoints were still usable. - MapOperation.removeByIndexRange(AerospikeConstants.FIRE_HISTORY, - fireHistoryEntries, MapReturnType.NONE)); - } - - private static boolean claim(final AtomicLong claimed, final long window) { - final long previous = claimed.get(); - return previous < window && claimed.compareAndSet(previous, window); - } - - private AtomicLong[] newClaims(final MagazineContext context) { - final AtomicLong[] claims = new AtomicLong[context.getShards()]; - Arrays.setAll(claims, index -> new AtomicLong(Long.MIN_VALUE)); - return claims; + return fireHistory.history(context); } /** @@ -450,6 +260,14 @@ public static com.phonepe.magazine.exception.MagazineException missingMetadata(f context.getMagazineIdentifier(), Objects.isNull(shard) ? 0 : shard)); } + /** Unconsumed slots exist and some hold data. */ + private static boolean isActive(final Record pointers, final Record counters) { + return Objects.nonNull(pointers) + && Objects.nonNull(counters) + && counters.getLong(AerospikeConstants.LOAD_COUNTER) > counters.getLong(AerospikeConstants.FIRE_COUNTER) + && pointers.getLong(AerospikeConstants.LOAD_POINTER) > pointers.getLong(AerospikeConstants.FIRE_POINTER); + } + private void adjustCounter(final MagazineContext context, final Integer shard, final StorageOperation operation, @@ -475,6 +293,24 @@ private Record operateOnMetadata(final Key key, }); } + /** Metadata bins plus fire history's refresh operation, if any. */ + private static Operation[] refreshOperations(final FireHistoryStore fireHistory) { + final List operations = new ArrayList<>(); + for (String bin : AerospikeConstants.getMetadataBins()) { + operations.add(Operation.get(bin)); + } + fireHistory.refreshReadOperation().ifPresent(operations::add); + return operations.toArray(new Operation[0]); + } + + private Record[] refreshRead(final MagazineContext context) { + return retryerFactory.call(() -> { + metrics.aerospikeCall(context.getMagazineIdentifier(), StorageOperation.REFRESH_ACTIVE_SHARDS); + return client.get(client.getBatchPolicyDefault(), + keys(context).pointerKeys().toArray(new Key[0]), refreshOperations); + }); + } + private Record[] batchRead(final MagazineContext context, final List keys, final StorageOperation operation) { diff --git a/magazine-core/src/main/java/com/phonepe/magazine/metrics/MagazineMetrics.java b/magazine-core/src/main/java/com/phonepe/magazine/metrics/MagazineMetrics.java index edcf8b0..8770209 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/metrics/MagazineMetrics.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/metrics/MagazineMetrics.java @@ -34,7 +34,7 @@ * {@link #aerospikeCall} counts round trips rather than logical operations, which is the * only way to see a dequeue's cost change under contention. *

- * Written to allocate nothing on the hot path: meters are resolved once per magazine into a + * Written to allocate nothing on the hot path: meters are resolved on first use into a * {@link Meters} holder indexed by enum ordinal, so recording is a map lookup plus an array index. * Percentile histograms are deliberately not enabled - they allocate hundreds of buckets per timer * and do bucket arithmetic per record. Add a {@link io.micrometer.core.instrument.config.MeterFilter} @@ -61,7 +61,7 @@ public final class MagazineMetrics { private static final String TAG_OUTCOME = "outcome"; private static final String TAG_OPERATION = "operation"; - /** Lets {@link Meters#outcomes} register any outcome enum without knowing which one it has. */ + /** Lets {@link Meters#outcome} register any outcome enum without knowing which one it has. */ interface Tagged { String getTag(); @@ -119,27 +119,31 @@ public MagazineMetrics(final MeterRegistry registry) { } public void aerospikeCall(final String magazineIdentifier, final StorageOperation operation) { - meters(magazineIdentifier).calls[operation.ordinal()].increment(); + meters(magazineIdentifier).call(operation).increment(); } public void fireClaim(final String magazineIdentifier, final ClaimOutcome outcome) { - meters(magazineIdentifier).fireClaims[outcome.ordinal()].increment(); + final Meters meters = meters(magazineIdentifier); + meters.outcome(meters.fireClaims, FIRE_CLAIMS, outcome).increment(); } public void fireHoleSkip(final String magazineIdentifier) { - meters(magazineIdentifier).fireHoleSkips.increment(); + meters(magazineIdentifier).fireHoleSkips().increment(); } public void fireOutcome(final String magazineIdentifier, final FireOutcome outcome) { - meters(magazineIdentifier).fireOutcomes[outcome.ordinal()].increment(); + final Meters meters = meters(magazineIdentifier); + meters.outcome(meters.fireOutcomes, FIRE_OUTCOMES, outcome).increment(); } public void loadOutcome(final String magazineIdentifier, final LoadOutcome outcome) { - meters(magazineIdentifier).loadOutcomes[outcome.ordinal()].increment(); + final Meters meters = meters(magazineIdentifier); + meters.outcome(meters.loadOutcomes, LOAD_OUTCOMES, outcome).increment(); } public void dedupeOutcome(final String magazineIdentifier, final DedupeOutcome outcome) { - meters(magazineIdentifier).dedupeOutcomes[outcome.ordinal()].increment(); + final Meters meters = meters(magazineIdentifier); + meters.outcome(meters.dedupeOutcomes, DEDUPE_OUTCOMES, outcome).increment(); } /** A primitive rather than a {@code Timer.Sample}, so timing allocates nothing. */ @@ -148,12 +152,12 @@ public long startNanos() { } public void recordFire(final long startNanos, final String magazineIdentifier) { - meters(magazineIdentifier).fireLatency + meters(magazineIdentifier).fireLatency() .record(System.nanoTime() - startNanos, TimeUnit.NANOSECONDS); } public void recordLoad(final long startNanos, final String magazineIdentifier) { - meters(magazineIdentifier).loadLatency + meters(magazineIdentifier).loadLatency() .record(System.nanoTime() - startNanos, TimeUnit.NANOSECONDS); } @@ -161,46 +165,69 @@ private Meters meters(final String magazineIdentifier) { return byMagazine.computeIfAbsent(magazineIdentifier, id -> new Meters(registry, id)); } + /** + * One magazine's meters, registered on first use. A racing first use resolves to the same + * meter in the registry. No allocation once registered. + */ private static final class Meters { - private final Counter[] calls; - private final Counter[] fireClaims; - private final Counter[] fireOutcomes; - private final Counter[] loadOutcomes; - private final Counter[] dedupeOutcomes; - private final Counter fireHoleSkips; - private final Timer fireLatency; - private final Timer loadLatency; + private final MeterRegistry registry; + private final Tags magazine; + private final Counter[] calls = new Counter[StorageOperation.values().length]; + private final Counter[] fireClaims = new Counter[ClaimOutcome.values().length]; + private final Counter[] fireOutcomes = new Counter[FireOutcome.values().length]; + private final Counter[] loadOutcomes = new Counter[LoadOutcome.values().length]; + private final Counter[] dedupeOutcomes = new Counter[DedupeOutcome.values().length]; + private Counter fireHoleSkips; + private Timer fireLatency; + private Timer loadLatency; private Meters(final MeterRegistry registry, final String magazineIdentifier) { - final Tags magazine = Tags.of(TAG_MAGAZINE, magazineIdentifier); - final StorageOperation[] operations = StorageOperation.values(); - this.calls = new Counter[operations.length]; - for (StorageOperation operation : operations) { - this.calls[operation.ordinal()] = Counter.builder(AEROSPIKE_CALLS) + this.registry = registry; + this.magazine = Tags.of(TAG_MAGAZINE, magazineIdentifier); + } + + Counter call(final StorageOperation operation) { + Counter counter = calls[operation.ordinal()]; + if (Objects.isNull(counter)) { + counter = Counter.builder(AEROSPIKE_CALLS) .tags(magazine.and(TAG_OPERATION, operation.getTag())) .register(registry); + calls[operation.ordinal()] = counter; } - this.fireClaims = outcomes(registry, FIRE_CLAIMS, magazine, ClaimOutcome.values()); - this.fireOutcomes = outcomes(registry, FIRE_OUTCOMES, magazine, FireOutcome.values()); - this.loadOutcomes = outcomes(registry, LOAD_OUTCOMES, magazine, LoadOutcome.values()); - this.dedupeOutcomes = outcomes(registry, DEDUPE_OUTCOMES, magazine, DedupeOutcome.values()); - this.fireHoleSkips = Counter.builder(FIRE_HOLE_SKIPS).tags(magazine).register(registry); - this.fireLatency = Timer.builder(FIRE_LATENCY).tags(magazine).register(registry); - this.loadLatency = Timer.builder(LOAD_LATENCY).tags(magazine).register(registry); + return counter; } - private static & Tagged> Counter[] outcomes(final MeterRegistry registry, - final String name, - final Tags magazine, - final E[] values) { - final Counter[] counters = new Counter[values.length]; - for (E outcome : values) { - counters[outcome.ordinal()] = Counter.builder(name) + & Tagged> Counter outcome(final Counter[] slots, final String name, final E outcome) { + Counter counter = slots[outcome.ordinal()]; + if (Objects.isNull(counter)) { + counter = Counter.builder(name) .tags(magazine.and(TAG_OUTCOME, outcome.getTag())) .register(registry); + slots[outcome.ordinal()] = counter; + } + return counter; + } + + Counter fireHoleSkips() { + if (Objects.isNull(fireHoleSkips)) { + fireHoleSkips = Counter.builder(FIRE_HOLE_SKIPS).tags(magazine).register(registry); + } + return fireHoleSkips; + } + + Timer fireLatency() { + if (Objects.isNull(fireLatency)) { + fireLatency = Timer.builder(FIRE_LATENCY).tags(magazine).register(registry); + } + return fireLatency; + } + + Timer loadLatency() { + if (Objects.isNull(loadLatency)) { + loadLatency = Timer.builder(LOAD_LATENCY).tags(magazine).register(registry); } - return counters; + return loadLatency; } } } diff --git a/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java b/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java index 641fabf..edd3a51 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/metrics/StorageOperation.java @@ -52,7 +52,7 @@ public enum StorageOperation { DELETE_DATA("delete_data"), BATCH_READ_DATA("batch_read_data"), BATCH_DELETE_DATA("batch_delete_data"), - /** The checkpoint write; one per shard per window, not one per claim. */ + /** Checkpoint write: one batch call per magazine, for shards whose pointer moved. */ WRITE_FIRE_HISTORY("write_fire_history"), /** The batch read behind {@code firePointerBefore}. */ READ_FIRE_HISTORY("read_fire_history"), diff --git a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java index 74f04f3..94a98bb 100644 --- a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java +++ b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineFireHistoryTest.java @@ -24,6 +24,7 @@ import com.phonepe.magazine.exception.ErrorCode; import com.phonepe.magazine.exception.MagazineException; import com.phonepe.magazine.impl.aerospike.common.AerospikeConstants; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import org.awaitility.Awaitility; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; @@ -68,6 +69,7 @@ void disabledByDefault() { void recordsOneCheckpointPerWindow() { final int shards = 4; // A wide window so the whole test runs inside one, making "same window" unambiguous. + awaitFreshWindow(60); final Magazine magazine = magazine("HIST_ONE", "HIST_ONE_SET", true, shards, 8, 60); loadAndFire(magazine, 3); @@ -146,14 +148,32 @@ void returnsNewestCheckpointNotAfterTheInstant() { loadAndFire(magazine, 4); magazine.firePointerBefore(Instant.now()); - final FireCheckpoint checkpoint = magazine.firePointerBefore(afterFirstBurst).get("SHARD_0"); + // Checkpoints are labelled with the end of the window they were observed in, so the first + // burst's checkpoint becomes answerable one window after it was taken - and the second + // burst, which started later still, cannot be. + final Instant question = afterFirstBurst.plusSeconds(WINDOW_SECONDS); + final FireCheckpoint checkpoint = magazine.firePointerBefore(question).get("SHARD_0"); assertNotNull(checkpoint, "the first burst's window is old enough to answer with"); - assertFalse(checkpoint.recordedAt().isAfter(afterFirstBurst), + assertFalse(checkpoint.recordedAt().isAfter(question), "the answer must come from a window that had already closed by then"); assertTrue(checkpoint.firePointer() <= pointerAfterFirstBurst, "the second burst's pointer must not be used to answer an earlier question"); } + @Test + @DisplayName("a checkpoint's label never precedes the moment its pointer was observed") + void labelNeverPrecedesObservation() { + final Magazine magazine = magazine("HIST_LABEL", "HIST_LABEL_SET", true, 1, 8); + loadAndFire(magazine, 2); + final long beforeRecording = System.currentTimeMillis(); + magazine.firePointerBefore(Instant.now()); + + // Labelling with the window start would claim the pointer was reached before it was read, + // which over-reports by however far into the window the write happened. + assertTrue(-newestWindowKey(magazine) >= beforeRecording, + "the newest label must be at or after the observation it records"); + } + @Test @DisplayName("a young magazine reports nothing rather than failing") void youngHistoryReturnsAbsent() { @@ -187,24 +207,83 @@ void evictedHistoryFailsLoudly() { @Test @DisplayName("reading advances history, so a burst that is never fired from again stays reachable") void readingRecordsACheckpoint() { - final Magazine magazine = magazine("HIST_BURST", "HIST_BURST_SET", true, 1, 8); + // Refresh effectively off, so only the read can be the one recording. + final Magazine magazine = magazine("HIST_BURST", "HIST_BURST_SET", true, 1, 8, + WINDOW_SECONDS, 3600, null); loadAndFire(magazine, 3); + final long firePointer = metadataRecord(magazine, 0).getLong(AerospikeConstants.FIRE_POINTER); + + awaitNextWindow(); + // No load, no fire: the magazine is idle. Reading alone must bring history up to the + // current pointer, otherwise a magazine that took one burst and was then abandoned would + // hold a checkpoint predating the burst and its orphans would never become visible. magazine.firePointerBefore(Instant.now()); - final long newestBefore = newestWindowKey(magazine); + assertEquals(firePointer, history(magazine, 0).get(newestWindowKey(magazine)), + "the newest checkpoint should hold the pointer the burst left behind"); + } + + @Test + @DisplayName("an idle shard records nothing new, from the refresh or from reads") + void idleShardWritesNothing() { + final SimpleMeterRegistry registry = new SimpleMeterRegistry(); + final Magazine magazine = magazine("HIST_IDLE", "HIST_IDLE_SET", true, 1, 8, + WINDOW_SECONDS, 1, registry); + loadAndFire(magazine, 2); + // A refresh during the burst may have claimed this window with a mid-burst pointer, in + // which case the final pointer is recorded in the next window. Settle that first. + magazine.firePointerBefore(Instant.now()); awaitNextWindow(); - // No load, no fire: the magazine is idle. Reading alone must still move history forward, - // otherwise a magazine that took one burst and was then abandoned would hold a single - // checkpoint predating the burst and its orphans would never become visible. magazine.firePointerBefore(Instant.now()); + final int entries = history(magazine, 0).size(); + final double writes = writeCalls(registry); + + for (int window = 0; window < 3; window++) { + awaitNextWindow(); + magazine.firePointerBefore(Instant.now()); + } - // The property, not a count. The active-shard refresh records on its own schedule, so how - // many windows exist by this point is not something the test controls - under load the - // preceding loadAndFire can contribute one of its own. What the read must guarantee is that - // a window strictly newer than anything present beforehand now exists. - assertTrue(newestWindowKey(magazine) < newestBefore, - "the read should have recorded a newer window by itself; keys are negated window " - + "timestamps, so newest sorts lowest"); + // The fire pointer only moves forward, so a checkpoint that already holds the current + // value stays true for every later instant. Rewriting it would cost a call per shard per + // window per process for magazines that are doing nothing at all. + assertEquals(entries, history(magazine, 0).size(), "no new checkpoint for an unchanged pointer"); + assertEquals(writes, writeCalls(registry), "no write round trip for an unchanged pointer"); + } + + @Test + @DisplayName("a second process skips a window another process already recorded") + void secondProcessSkipsARecordedWindow() { + awaitFreshWindow(60); + final Magazine first = magazine("HIST_TWO", "HIST_TWO_SET", true, 1, 8, 60, 3600, null); + loadAndFire(first, 3); + first.firePointerBefore(Instant.now()); + + final SimpleMeterRegistry registry = new SimpleMeterRegistry(); + final Magazine second = magazine("HIST_TWO", "HIST_TWO_SET", true, 1, 8, 60, 3600, registry); + second.firePointerBefore(Instant.now()); + + assertEquals(0.0, writeCalls(registry), + "the window's checkpoint is already on the record, so the second process must not call"); + } + + @Test + @DisplayName("every shard's checkpoint goes out in one batch call") + void checkpointsAreBatched() { + final int shards = 8; + final SimpleMeterRegistry registry = new SimpleMeterRegistry(); + awaitFreshWindow(60); + final Magazine magazine = magazine("HIST_BATCH", "HIST_BATCH_SET", true, shards, 8, + 60, 3600, registry); + for (int i = 0; i < 40; i++) { + magazine.load("message-" + random.nextLong()); + } + final int withRecords = shardsWithMetadata(magazine, shards); + assertTrue(withRecords > 1, "expected several shards to have been written to"); + + magazine.firePointerBefore(Instant.now()); + + assertEquals(withRecords, totalCheckpoints(magazine, shards)); + assertEquals(1.0, writeCalls(registry), "one round trip for the whole magazine, not one per shard"); } @Test @@ -218,7 +297,8 @@ void firingRecordsCheckpoints() { Awaitility.await().atMost(Duration.ofSeconds(20)).pollInterval(Duration.ofMillis(200)) .untilAsserted(() -> { loadAndFire(magazine, 1); - assertFalse(history(magazine, 0).isEmpty(), + final Map history = history(magazine, 0); + assertTrue(history != null && !history.isEmpty(), "firing alone should eventually record a checkpoint"); }); } @@ -228,6 +308,7 @@ void firingRecordsCheckpoints() { void lateShardIsCheckpointedInTheSameWindow() { final int shards = 8; // One window wide enough to hold the whole test, so nothing here can roll into the next. + awaitFreshWindow(60); final Magazine magazine = magazine("HIST_LATE", "HIST_LATE_SET", true, shards, 8, 60); loadAndFire(magazine, 1); magazine.firePointerBefore(Instant.now()); @@ -313,12 +394,26 @@ private Magazine magazine(final String identifier, final String setName, private Magazine magazine(final String identifier, final String setName, final boolean enabled, final int shards, final int entries, final int windowSeconds) { + return magazine(identifier, setName, enabled, shards, entries, windowSeconds, 1, null); + } + + private Magazine magazine(final String identifier, final String setName, + final boolean enabled, final int shards, final int entries, + final int windowSeconds, final int refreshSeconds, + final SimpleMeterRegistry registry) { return Magazine.builder() .magazineIdentifier(identifier) - .baseMagazineStorage(storageWithHistory(setName, enabled, windowSeconds, entries, shards)) + .baseMagazineStorage(buildStorage(config(setName, enabled, windowSeconds, entries, shards, + refreshSeconds), + String.class, false, "FARM_ID", "CLIENT_ID", MagazineScope.LOCAL, aerospikeClient, + registry)) .build(); } + private static double writeCalls(final SimpleMeterRegistry registry) { + return counterValue(registry, "magazine.aerospike.calls", "operation", "write_fire_history"); + } + private AerospikeStorage storageWithHistory(final String setName, final boolean enabled, final int windowSeconds, final int entries) { return storageWithHistory(setName, enabled, windowSeconds, entries, 1); @@ -327,7 +422,14 @@ private AerospikeStorage storageWithHistory(final String setName, final private AerospikeStorage storageWithHistory(final String setName, final boolean enabled, final int windowSeconds, final int entries, final int shards) { - return buildStorage(AerospikeStorageConfig.builder() + return buildStorage(config(setName, enabled, windowSeconds, entries, shards, 1), + String.class, false, "FARM_ID", "CLIENT_ID", MagazineScope.LOCAL, aerospikeClient); + } + + private static AerospikeStorageConfig config(final String setName, final boolean enabled, + final int windowSeconds, final int entries, + final int shards, final int refreshSeconds) { + return AerospikeStorageConfig.builder() .namespace(NAMESPACE) .dataSetName(setName) .metaSetName("META_SET") @@ -335,12 +437,25 @@ private AerospikeStorage storageWithHistory(final String setName, final .metaDataTtl(2 * 30 * 24 * 60 * 60) .shards(shards) // Refresh often, so the fire path exercises the recorder within a test's life. - .activeShardRefreshSeconds(1) + .activeShardRefreshSeconds(refreshSeconds) .fireHistoryEnabled(enabled) .fireHistoryWindowSeconds(windowSeconds) .fireHistoryEntries(entries) - .build(), - String.class, false, "FARM_ID", "CLIENT_ID", MagazineScope.LOCAL, aerospikeClient); + .build(); + } + + /** + * Windows are aligned to the clock, so a test that must stay inside one window starts near the + * beginning of one rather than wherever the clock happens to be. + */ + private static void awaitFreshWindow(final int windowSeconds) { + final long windowMillis = windowSeconds * 1_000L; + final long remaining = windowMillis - System.currentTimeMillis() % windowMillis; + // These tests take a couple of seconds; fifteen leaves ample margin. + if (remaining < 15_000) { + Awaitility.await("the start of a fresh window").pollDelay(Duration.ofMillis(remaining + 50)) + .atMost(Duration.ofMillis(remaining + 5_000)).until(() -> true); + } } private void awaitNextWindow() { diff --git a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineMetricsTest.java b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineMetricsTest.java index d36563f..8961aab 100644 --- a/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineMetricsTest.java +++ b/magazine-core/src/test/java/com/phonepe/magazine/impl/aerospike/MagazineMetricsTest.java @@ -134,4 +134,28 @@ void metricsCanBeDisabledExplicitlyEvenWithARegistry() { assertEquals(0.0, counterValue(registry, "magazine.aerospike.calls", "operation", "write_data")); assertTrue(registry.getMeters().isEmpty(), "disabled metrics must register no meters"); } + + /** + * A meter appears the first time it is recorded. Registering every operation and outcome up + * front meant thirty meters per magazine, most of them zero for the life of the process. + */ + @Test + void onlyMetersThatHaveRecordedSomethingAreRegistered() { + SimpleMeterRegistry registry = new SimpleMeterRegistry(); + AerospikeStorage storage = buildStorage( + buildStorageConfig("NAMESPACE", "DATA_SET", "META_SET", + 30 * 24 * 60 * 60, 2 * 30 * 24 * 60 * 60, 1), + String.class, false, "FARM_ID", "CLIENT_ID", MagazineScope.LOCAL, aerospikeClient, + registry); + MagazineContext context = storage.initialize("LAZY_METERS_MAGAZINE"); + + assertTrue(storage.load(context, "PAYLOAD")); + + assertEquals(1.0, counterValue(registry, "magazine.aerospike.calls", "operation", "write_data")); + assertTrue(registry.find("magazine.aerospike.calls").tag("operation", "claim_fire_pointer") + .counters().isEmpty(), "nothing has been fired, so the claim counter must not exist"); + assertTrue(registry.find("magazine.fire.latency").timers().isEmpty()); + assertTrue(registry.find("magazine.dedupe.outcomes").counters().isEmpty()); + assertTrue(registry.find("magazine.load.outcomes").tag("outcome", "failed").counters().isEmpty()); + } } From fa922c84dea85897297c56e652470b4a75f4f9e3 Mon Sep 17 00:00:00 2001 From: Shantanu Tiwari Date: Thu, 15 Oct 2026 17:47:58 +0530 Subject: [PATCH 08/10] Sonar cleanup --- .../aerospike/store/FireHistoryStore.java | 40 ++++++++++--------- .../store/MagazineMetadataStore.java | 11 ++--- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java index dad52d7..48dddf7 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java @@ -132,7 +132,7 @@ public void recordOnRefresh(final MagazineContext context, final Record[] pointe final long now = System.currentTimeMillis(); final Window window = Window.containing(now, windowMillis); if (now >= window.start() + state(context).getJitterMillis()) { - record(context, pointerRecords, window); + recordCheckpoints(context, pointerRecords, window); } } @@ -148,7 +148,7 @@ public Map firePointerBefore(final MagazineContext conte final Record[] records = readHistory(context); // Record first (undelayed) so history still advances even if resolving throws. - record(context, records, Window.containing(System.currentTimeMillis(), windowMillis)); + recordCheckpoints(context, records, Window.containing(System.currentTimeMillis(), windowMillis)); final Map answers = new HashMap<>(capacityFor(context.getShards())); for (int shard = 0; shard < context.getShards(); shard++) { @@ -177,29 +177,30 @@ public Map> history(final MagazineContext context) // --- recording --------------------------------------------------------------------------- - private void record(final MagazineContext context, final Record[] pointerRecords, final Window window) { + private void recordCheckpoints(final MagazineContext context, final Record[] pointerRecords, + final Window window) { final MagazineState state = state(context); - final List shards = shardsNeedingCheckpoint(context, pointerRecords, window, state); - if (!shards.isEmpty()) { - write(context, pointerRecords, shards, window, state); + final List pending = pendingCheckpoints(context, pointerRecords, window, state); + if (!pending.isEmpty()) { + write(context, pending, window, state); } } /** Shards that need this window's checkpoint, each already claimed for this process. */ - private static List shardsNeedingCheckpoint(final MagazineContext context, + private static List pendingCheckpoints(final MagazineContext context, final Record[] pointerRecords, final Window window, final MagazineState state) { - final List shards = new ArrayList<>(); + final List pending = new ArrayList<>(); for (int shard = 0; shard < context.getShards(); shard++) { final Record pointers = pointerRecords[shard]; if (Objects.nonNull(pointers) && needsCheckpoint(pointers, window) && state.claim(shard, window)) { - shards.add(shard); + pending.add(new PendingCheckpoint(shard, firePointer(pointers))); } } - return shards; + return pending; } private static boolean needsCheckpoint(final Record pointers, final Window window) { @@ -215,21 +216,20 @@ private static boolean needsCheckpoint(final Record pointers, final Window windo /** One batch call for every shard; a failed shard's claim is released so it is retried. */ private void write(final MagazineContext context, - final Record[] pointerRecords, - final List shards, + final List pending, final Window window, final MagazineState state) { final List keys = pointerKeys.apply(context); - final List writes = new ArrayList<>(shards.size()); - for (int shard : shards) { - writes.add(checkpointWrite(keys.get(shard), window.label(), firePointer(pointerRecords[shard]))); - } + final List writes = pending.stream() + .map(checkpoint -> checkpointWrite(keys.get(checkpoint.shard()), window.label(), + checkpoint.firePointer())) + .toList(); try { metrics.aerospikeCall(context.getMagazineIdentifier(), StorageOperation.WRITE_FIRE_HISTORY); client.operate(batchPolicy, writes); } catch (Exception e) { - shards.forEach(shard -> state.release(shard, window)); + pending.forEach(checkpoint -> state.release(checkpoint.shard(), window)); log.warn("Could not record fire history for magazine {} window {}.", context.getMagazineIdentifier(), window.start(), e); return; @@ -240,7 +240,7 @@ private void write(final MagazineContext context, if (resultCode == ResultCode.OK) { continue; } - final int shard = shards.get(i); + final int shard = pending.get(i).shard(); state.release(shard, window); // A missing record is expected: the shard has none yet, or it expired. if (resultCode != ResultCode.KEY_NOT_FOUND_ERROR) { @@ -342,6 +342,10 @@ static Window containing(final long now, final long width) { } } + /** A shard to checkpoint, with the fire pointer observed on it. */ + private record PendingCheckpoint(int shard, long firePointer) { + } + /** By {@code label}, the fire pointer had reached at least {@code firePointer}. */ private record Checkpoint(long label, long firePointer) { diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java index ec3a873..68e5b2d 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/MagazineMetadataStore.java @@ -39,7 +39,6 @@ import com.phonepe.magazine.metrics.MagazineMetrics; import com.phonepe.magazine.metrics.StorageOperation; import java.time.Instant; -import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; @@ -47,6 +46,7 @@ import java.util.Objects; import java.util.OptionalLong; import java.util.concurrent.ConcurrentHashMap; +import java.util.stream.Stream; /** * Reads and writes the per-shard metadata records: load/fire pointers and load/fire counters. @@ -295,12 +295,9 @@ private Record operateOnMetadata(final Key key, /** Metadata bins plus fire history's refresh operation, if any. */ private static Operation[] refreshOperations(final FireHistoryStore fireHistory) { - final List operations = new ArrayList<>(); - for (String bin : AerospikeConstants.getMetadataBins()) { - operations.add(Operation.get(bin)); - } - fireHistory.refreshReadOperation().ifPresent(operations::add); - return operations.toArray(new Operation[0]); + return Stream.concat(Arrays.stream(AerospikeConstants.getMetadataBins()).map(Operation::get), + fireHistory.refreshReadOperation().stream()) + .toArray(Operation[]::new); } private Record[] refreshRead(final MagazineContext context) { From 8e9072c049e044151210f387dbe46e5c68f97d7d Mon Sep 17 00:00:00 2001 From: Shantanu Tiwari Date: Thu, 15 Oct 2026 17:58:07 +0530 Subject: [PATCH 09/10] Sonar fix --- .../impl/aerospike/store/FireHistoryStore.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java index 48dddf7..b9bc4fd 100644 --- a/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java +++ b/magazine-core/src/main/java/com/phonepe/magazine/impl/aerospike/store/FireHistoryStore.java @@ -153,7 +153,7 @@ public Map firePointerBefore(final MagazineContext conte final Map answers = new HashMap<>(capacityFor(context.getShards())); for (int shard = 0; shard < context.getShards(); shard++) { final int current = shard; - resolve(context, current, checkpoints(records[current]), instant) + resolve(context, current, checkpoints(historyBin(records[current])), instant) .ifPresent(checkpoint -> answers.put(context.shardId(current), checkpoint.toEntity())); } return answers; @@ -165,7 +165,7 @@ public Map> history(final MagazineContext context) final Record[] records = readHistory(context); final Map> history = new HashMap<>(capacityFor(context.getShards())); for (int shard = 0; shard < context.getShards(); shard++) { - final List checkpoints = checkpoints(records[shard]).stream() + final List checkpoints = checkpoints(historyBin(records[shard])).stream() .map(Checkpoint::toEntity) .toList(); if (!checkpoints.isEmpty()) { @@ -204,7 +204,8 @@ && needsCheckpoint(pointers, window) } private static boolean needsCheckpoint(final Record pointers, final Window window) { - final Optional newest = checkpoints(pointers).stream().findFirst(); + final Optional newest = checkpoints(pointers.getValue(AerospikeConstants.FIRE_HISTORY)) + .stream().findFirst(); if (newest.isEmpty()) { return true; } @@ -299,9 +300,13 @@ private Optional resolve(final MagazineContext context, return answer; } + /** The history bin of a batch-read slot, which is null when the shard has no record. */ + private static Object historyBin(final Record pointerRecord) { + return Objects.isNull(pointerRecord) ? null : pointerRecord.getValue(AerospikeConstants.FIRE_HISTORY); + } + /** Checkpoints newest first, from either the full map or the refresh read's single entry. */ - private static List checkpoints(final Record pointerRecord) { - final Object raw = Objects.isNull(pointerRecord) ? null : pointerRecord.getValue(AerospikeConstants.FIRE_HISTORY); + private static List checkpoints(final Object raw) { final Stream rawEntries; if (raw instanceof Map map) { rawEntries = map.entrySet().stream(); From a0a36d80bc401e7dbf1ba681fd193a00e67bab79 Mon Sep 17 00:00:00 2001 From: Shantanu Tiwari Date: Thu, 15 Oct 2026 17:59:08 +0530 Subject: [PATCH 10/10] Version bump --- magazine-core/pom.xml | 2 +- magazine-coverage/pom.xml | 2 +- magazine-dw-bundle/pom.xml | 2 +- pom.xml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/magazine-core/pom.xml b/magazine-core/pom.xml index 2e93619..fb4c23e 100644 --- a/magazine-core/pom.xml +++ b/magazine-core/pom.xml @@ -7,7 +7,7 @@ com.phonepe magazine - 2.0.1-SNAPSHOT + 2.0.2-SNAPSHOT magazine-core diff --git a/magazine-coverage/pom.xml b/magazine-coverage/pom.xml index 749c42a..732275d 100644 --- a/magazine-coverage/pom.xml +++ b/magazine-coverage/pom.xml @@ -7,7 +7,7 @@ com.phonepe magazine - 2.0.1-SNAPSHOT + 2.0.2-SNAPSHOT magazine-coverage diff --git a/magazine-dw-bundle/pom.xml b/magazine-dw-bundle/pom.xml index a304fbd..bd65519 100644 --- a/magazine-dw-bundle/pom.xml +++ b/magazine-dw-bundle/pom.xml @@ -7,7 +7,7 @@ com.phonepe magazine - 2.0.1-SNAPSHOT + 2.0.2-SNAPSHOT magazine-dw-bundle diff --git a/pom.xml b/pom.xml index 710b983..4bf6755 100644 --- a/pom.xml +++ b/pom.xml @@ -7,7 +7,7 @@ com.phonepe magazine - 2.0.1-SNAPSHOT + 2.0.2-SNAPSHOT pom Magazine