Skip to content

feat: support nats ttl marker #8377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
hengfeiyang merged 7 commits into from
Sep 12, 2025
Merged

feat: support nats ttl marker #8377

hengfeiyang merged 7 commits into from
Sep 12, 2025

Conversation

@hengfeiyang
Copy link
Contributor

@hengfeiyang hengfeiyang commented Sep 11, 2025 •
edited by github-actions bot
Loading

User description

impl #8367

PR Type

Enhancement

Description

  • Add NATS v2.11 support flag

  • Set KV marker TTL for nodes/clusters

  • Upgrade async-nats to 0.42

  • Minor test cleanup

Diagram Walkthrough

flowchart LR
  Config["Config: add `v211_support` flag"] -- controls --> NatsKV["NATS KV bucket creation"]
  NatsKV -- sets --> MaxAge["`bucket.max_age` from node TTL"]
  NatsKV -- optional --> LimitMarkers["`bucket.limit_markers = ttl` (v2.11)"]
  Cargo["Dependency: async-nats 0.42"] -- enables --> NatsKV
Loading

File Walkthrough
Relevant files
Enhancement
config.rs
Introduce NATS v2.11 support configuration                             

src/config/src/config.rs

  • Add v211_support boolean config with env ZO_NATS_V211_SUPPORT.
  • Document support for NATS v2.11.x.
 +6/-0     
nats.rs
Add KV marker TTL for nodes/clusters                                         

src/infra/src/db/nats.rs

  • Set bucket.max_age from node heartbeat TTL.
  • Conditionally set bucket.limit_markers to TTL when v211_support is
    enabled.
 +5/-1     
Miscellaneous
file.rs
Clean up unused test imports                                                         

src/config/src/utils/file.rs

  • Remove unused chrono imports in tests.
 +0/-1     
Dependencies
Cargo.toml
Upgrade async-nats dependency                                                       

Cargo.toml

  • Bump async-nats from 0.39 to 0.42.
 +1/-1     

@github-actions
Copy link
Contributor

github-actions bot commented Sep 11, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate and clamp TTL

Guard against zero or excessively small ttl values that can cause immediate
expiration or marker churn. Enforce a sensible minimum duration before applying it
to max_age and limit_markers.

src/infra/src/db/nats.rs [89-93]

-let ttl = Duration::from_secs(cfg.limit.node_heartbeat_ttl as u64);
+let mut ttl = Duration::from_secs(cfg.limit.node_heartbeat_ttl as u64);
+// Enforce a sensible minimum TTL (e.g., 1 second) to avoid immediate expiry/marker churn.
+if ttl.is_zero() {
+    ttl = Duration::from_secs(1);
+}
 bucket.max_age = ttl;
 if cfg.nats.v211_support {
     bucket.limit_markers = Some(ttl);
 }
Suggestion importance[1-10]: 5

__

Why: Clamping a zero TTL to a minimum avoids immediate expiry; it's a reasonable safety improvement, though the PR does not indicate zero is possible. The code mapping is accurate and the improved code reflects the described change.

Low
General
Avoid setting unsupported markers

Only set limit_markers when the server actually supports it to avoid failures on
older NATS versions. Consider gating it behind a runtime capability check or feature
negotiation rather than a static flag.

src/infra/src/db/nats.rs [91-93]

 if cfg.nats.v211_support {
+    // Only set when supported to prevent server-side errors on older NATS.
     bucket.limit_markers = Some(ttl);
+} else {
+    // Ensure it's not set when unsupported.
+    bucket.limit_markers = None;
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion is sensible in spirit but the improved code still relies on the same static flag and adds a redundant else branch; runtime capability detection is not implemented here. Impact is modest and partially redundant.

Low

greptile-apps[bot]
greptile-apps bot reviewed Sep 11, 2025
View reviewed changes
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR implements support for NATS KV TTL markers (issue #8367) by upgrading the async-nats dependency and adding conditional feature support. The changes introduce a new configuration flag v211_support that enables NATS to generate events when key-value entries expire, specifically for 'nodes' and 'clusters' buckets used in cluster management.

The implementation consists of four main changes:

  1. Configuration: Added ZO_NATS_V211_SUPPORT environment variable (defaults to false) to control the new functionality
  2. Dependency upgrade: Updated async-nats from version 0.39 to 0.42 to access the limit_markers field
  3. NATS integration: Modified the bucket creation logic to conditionally set limit_markers with the same TTL as max_age when v211_support is enabled
  4. Code cleanup: Removed unused chrono imports from test files

The feature enables OpenObserve to receive expiration events from NATS instead of relying solely on client-side TTL checking. This is particularly valuable for cluster heartbeat monitoring where knowing when node entries expire is crucial for maintaining accurate cluster state. The implementation is backward compatible as it only applies the new functionality when explicitly enabled and only to specific bucket types that already have TTL configured.

Confidence score: 4/5

  • This PR is safe to merge with minimal risk as it's gated behind a feature flag
  • Score reflects well-structured conditional implementation but minor concern about dependency upgrade impact
  • Pay close attention to the NATS configuration logic in src/infra/src/db/nats.rs

4 files reviewed, no comments

Edit Code Review Bot Settings | Greptile

@hengfeiyang hengfeiyang merged commit b72e85e into main Sep 12, 2025
28 checks passed
@hengfeiyang hengfeiyang deleted the feat/nats-marker-ttl branch September 12, 2025 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Subhra264 Subhra264 Subhra264 approved these changes

@oasisk oasisk Awaiting requested review from oasisk

+1 more reviewer

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

✏️ Feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hengfeiyang @Subhra264