Skip to content

feat(processing_engine): Store triggers by DbId and TriggerId instead of name #26186

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kumarlokesh
Copy link

@kumarlokesh kumarlokesh commented Mar 22, 2025

Attempts to close ##26153

Describe your proposed changes here.

  1. Fixed type safety in PluginChannels struct:

    • Changed schedule_triggers type from HashMap<String, HashMap<String, mpsc::Sender<ScheduleEvent>>> to HashMap<(DbId, TriggerId), mpsc::Sender<ScheduleEvent>> to properly handle schedule events
  2. Improved trigger removal logic in remove_trigger method:

    • Removed incorrect attempt to get and remove from trigger maps
    • Now directly removing entries from HashMaps using correct keys
    • For WAL and schedule triggers: using tuple key (db_id, trigger_id)
    • For request triggers: using path string directly
  • I've read the contributing section of the project README.
  • Signed CLA (if not already signed).

@kumarlokesh kumarlokesh force-pushed the processing_engine/store-triggers-by-db-id-trigger-id branch from 0ee4edf to 8b2921a Compare March 22, 2025 16:09
@kumarlokesh
Copy link
Author

It seems the test cli::test_trigger_cache_cleanup is failing https://app.circleci.com/pipelines/github/influxdata/influxdb/44923/workflows/015cf4c0-526b-44db-8bfa-e5c806f56648/jobs/425503:

> ──── STDERR:             influxdb3::lib cli::test_trigger_cache_cleanup
> 
> thread 'cli::test_trigger_cache_cleanup' panicked at influxdb3/tests/cli/mod.rs:2548:5:
> assertion failed: `(left == right)`
> 
> Diff < left / right > :
>  Array [
>      Object {
> <        "count(*)": Number(2),
> >        "count(*)": Number(1),
>      },
>  ]

Need to investigate this further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant