Repository navigation
Feature/annotation cache - #3957
FineAndDandy wants to merge 29 commits into
Conversation
| Message<AnnotationMessage> message = MessageBuilder.withPayload(annotationMessage).setHeader(AMQP_CORRELATION_DATA_HEADER, correlationData) | ||
| .setHeader(AMQP_PUBLISH_CONFIRM_CORRELATION_HEADER, correlationData).build(); | ||
|
|
||
| log.info("Sending message synchronously, ID: {}", correlationId); |
There was a problem hiding this comment.
I see a lot of logging at INFO level, confirming this is intended
| throw new AnnotationStorageException("Broker rejected message (NACK). Reason: " + confirm.getReason()); | ||
| } | ||
| } catch (MessagingException | AmqpException e) { | ||
| log.info("caught messaging exception", e); |
| processor.process(missingEntry); | ||
| assertNull(missingEntry.getValue()); | ||
|
|
||
| Map.Entry<String,Set<String>> differentIdEntry = new AbstractMap.SimpleEntry<>(CACHE_KEY, Set.of("other")); |
|
|
||
| @Test | ||
| void federationLockWaitRejectsInvalidDurations() { | ||
| java.time.Duration[] invalidValues = {null, java.time.Duration.ZERO, java.time.Duration.ofNanos(-1), java.time.Duration.ofNanos(999_999)}; |
There was a problem hiding this comment.
no fully qualified imports
apmoriarty
left a comment
There was a problem hiding this comment.
A few non-blocking comments (preferences)
| annotationMapConfig.addEntryListenerConfig(listenerConfig); | ||
|
|
||
| MapConfig documentIndexConfig = config.getMapConfig(DOC_ANNOTATIONS_MAP); | ||
| documentIndexConfig.setTimeToLiveSeconds(0); |
There was a problem hiding this comment.
I think providing a merge-policy here that unions the set of ids may help prevent need for later reconciliation, although I think the reconciler will do the job.
| documentIndexConfig.setTimeToLiveSeconds(0); | |
| documentIndexConfig.setTimeToLiveSeconds(0); | |
| documentIndexConfig.setMergePolicyConfig( | |
| new MergePolicyConfig( | |
| UnionAnnotationIdsMergePolicy.class.getName(), | |
| MergePolicyConfig.DEFAULT_BATCH_SIZE)); |
There was a problem hiding this comment.
Example union merge policy config:
UnionAnnotationIdsMergePolicy.java
tharke13
left a comment
There was a problem hiding this comment.
Looks good, just the one suggestion about adding a merge policy.
tharke13
left a comment
There was a problem hiding this comment.
Approving since I think the custom merge policy is optional and could be added later.
|
|
||
| ## Purpose | ||
|
|
||
| This service provides the regional Hazelcast cache used by Sonicweb. Annotation entries are immutable and are keyed by annotation ID within a document-specific map: |
There was a problem hiding this comment.
| This service provides the regional Hazelcast cache used by Sonicweb. Annotation entries are immutable and are keyed by annotation ID within a document-specific map: | |
| This service provides the multi-site Hazelcast cache used by Sonicweb. Annotation entries are immutable and are keyed by annotation ID within a document-specific map named `doc-annotations`: |
|
|
||
| A local Sonicweb write is written to Hazelcast and published by `AnnotationMapStore` to RabbitMQ. RabbitMQ delivery is acknowledged before the write is considered accepted by this service; Accumulo persistence and regional federation are asynchronous consumers. | ||
|
|
||
| Federated messages are consumed locally only when their `region.id` differs from the configured `region.name`. The message must also contain `id.type`, because `Annotation` contains the document ID but not the Sonicweb identifier type. |
There was a problem hiding this comment.
I've often wondered whether Annotation should contain the identifier type. I'm not quite happy that it doesn't.
|
|
||
| ## Reconciliation | ||
|
|
||
| `AnnotationCacheReconciler` periodically treats the `annotations:*` maps as authoritative and unions missing annotation IDs into `doc-annotations`. Reconciliation is intentionally add-only: stale IDs are measured and logged, while normal expiration/removal listeners remain responsible for deleting them. Repairing an index also invalidates that document's fetch records. |
There was a problem hiding this comment.
I think I'm missing fundamental information about what the annotations:* maps contain and why it might be necessary to "union missing annotation ID's into doc-annotations. Perhaps you could provide additional background or explanation earlier on in this document? Could you explain why reconciliation is needed (e.g, X falls out of sync with Y because Z)
| } | ||
|
|
||
| ids.add(annotationId); | ||
| entry.setValue(ids); |
There was a problem hiding this comment.
Is it necessary to set the value every time, or could you move the setValue inside the null check or utilize computeIfAbsent here instead? I'm thinking this must be a hazelcastism, because I see similar behavior in RemoveAnnotationIdProcessor. If it's a hazelcastism, it might be worth explicitly calling this out briefly in a comment or javadoc.
| public static final String ANNOTATIONS_MAP = "annotations:"; | ||
| public static final String DOC_ANNOTATIONS_MAP = "doc-annotations"; | ||
| public static final String FETCH_MAP = "doc-fetch-record:"; |
There was a problem hiding this comment.
It might be cool to add minimal javadoc for each of these here, but perhaps if each of these is explained in the README in detail, it won't be necessary.
| } | ||
|
|
||
| @Override | ||
| protected Object convertToInternal(Object payload, org.springframework.messaging.MessageHeaders headers, Object conversionHint) { |
There was a problem hiding this comment.
Looks like a fully qualified class name snuck in here.
| } | ||
|
|
||
| public boolean send(Message<AnnotationMessage> message) { | ||
| return streamBridge.send("persisted-out-0", message); |
There was a problem hiding this comment.
Should persisted-out-0 be configurable or a constant instead of a raw string?
| import datawave.microservice.annotationCache.config.AnnotationCacheProperties; | ||
|
|
||
| /** | ||
| * Periodically repairs missing entries in the derived document annotation index. |
There was a problem hiding this comment.
I'm still not quite sure what the purpose of the derived document annotation index is based on the available documentation :)
| result.mapsScanned, result.missingIdsRepaired, result.staleIdsObserved, result.failures, | ||
| result.cycleComplete ? "" : "; more maps remain"); | ||
| } catch (HazelcastInstanceNotActiveException e) { | ||
| log.debug("Hazelcast stopped during annotation cache reconciliation", e); |
There was a problem hiding this comment.
It seems off to have a no-op debug message in an exception handler. Maybe this should be at least info?
| log.debug("Hazelcast stopped during annotation cache reconciliation", e); | |
| log.info("Hazelcast stopped during annotation cache reconciliation", e); |
| import datawave.microservice.annotationCache.api.RegionConfiguration; | ||
|
|
||
| @Component | ||
| public class AnnotationMapStore implements MapStore<String,Object>, HazelcastInstanceAware, MapLoaderLifecycleSupport { |
There was a problem hiding this comment.
A tiny bit of class Javadoc would be nice.
9c4afc4
|
I wanted to document the motivations for the pom changes to annotation-cache here. This was inspired/patterned off of the authorization service
|
|
#3964 is now merged, so it might be useful to rebase on that to pick up the |
There was a problem hiding this comment.
Consider integratingannotation-cache/api/README.md and annotation-cache/service/README.md into a single annotation-cache/README.md or providing a top level readme that references each?
|
|
||
| A fetch marker records a successful Datawave lookup for one authorization context. It does not guarantee that the annotation cache contains a complete snapshot. Its presence and TTL determine freshness; `FetchRecord` fields are diagnostic only. | ||
|
|
||
| Sonicweb writes a marker after source retrieval and cache merge succeed. Empty and not-found responses also receive markers. Errors do not. Cache hits do not renew markers, and expired markers cause a source lookup on the next request, not a background refresh. Because markers are written after retrieval, source latency adds to the age of the source observation. |
There was a problem hiding this comment.
I'm a little confused by the statement "Sonicweb writes a marker after source retrieval and cache merge succeed" compared to the following statement: "Empty and not-found responses also receive markers".
| Sonicweb writes a marker after source retrieval and cache merge succeed. Empty and not-found responses also receive markers. Errors do not. Cache hits do not renew markers, and expired markers cause a source lookup on the next request, not a background refresh. Because markers are written after retrieval, source latency adds to the age of the source observation. | |
| Sonicweb writes fetch markers for source lookups that return annotations, no results, or “not found.” It writes the marker after retrieval and cache merging finish without error; failures do not create markers. Serving cached results does not extend a marker’s lifetime. Once a marker expires, the next request queries the source again; expiration does not trigger a background refresh. The marker’s lifetime starts after retrieval finishes, so the source data may be older than the marker by the time spent retrieving it. |
| topology-poll-interval-ms: 5000 | ||
| ``` | ||
|
|
||
| Both TTLs must be positive whole seconds, and `max-fetch-age` must not exceed `max-cache-age`. Max-idle expiration is disabled. Federation lock wait and topology poll interval must be positive; topology settle delay may be zero. |
There was a problem hiding this comment.
If the configuration property was x-y-ttl, it would be fine to refer to them as TTLs in the documents, but it take a second more to parse this paragraph since it's not obvious for the attention starved developer to make the connection. Suggest a sligh modification
| Both TTLs must be positive whole seconds, and `max-fetch-age` must not exceed `max-cache-age`. Max-idle expiration is disabled. Federation lock wait and topology poll interval must be positive; topology settle delay may be zero. | |
| Both `max-cache-age` and `max-fetch-age` must be positive whole seconds. `max-fetch-age` must not exceed `max-cache-age`. Max-idle expiration is disabled, so a lack of activity does not cause entries to drop from the cache. `federation-lock-wait` wait and `topology-poll-interval-ms` must be positive; `topology-settle-delay` may be zero. |
|
|
||
| Both TTLs must be positive whole seconds, and `max-fetch-age` must not exceed `max-cache-age`. Max-idle expiration is disabled. Federation lock wait and topology poll interval must be positive; topology settle delay may be zero. | ||
|
|
||
| RabbitMQ bindings and connection settings are supplied separately through application configuration. Bind `persisted-out-0` and `loadCache-in-0` to `annotation`, use consumer group `cache`, and set the content type to `application/x-protobuf`. Publication requires correlated confirms and returns. The companion Helm configuration is `configuration/configMapFiles/annotation-cache.yml` in the `datawave-helm-charts` repository. |
There was a problem hiding this comment.
Take a look at the docker/config directory - there are sample configurations here for use in docker-compose that also serve as good exemplars for use elsewhere.
…er to user AnnotationMessage and process all varients of eviction. LoadCacheConsumer to use AnnotationMessage and avoid republishing to AnnotationMapStore
Upgrade datawave dependency to 8.1.0 Remove now redundant classes MessageConfig and SpecificProtobufConverterConfig
…unique maps for each document
… patterns similar to authorization service
5f56a69 to
bc90b9c
Compare
Adds new annotation-cache microservice service