-
Notifications
You must be signed in to change notification settings - Fork 74
Consolidated Ingest - Phase 5 (Supervision Tree/Worker) #3088
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
Conversation
…o-support-consolidated-ingest
…olidated backends in source supervisor
…lidated-ingestion
…lidated-ingestion
…gle implementation for now. This is temporary until PR#3089 is in place
| end | ||
|
|
||
| @impl GenStage | ||
| def format_discarded(discarded, %{consolidated: true} = state) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can consider removing this callback fully since we are not inserting directly into the Producer process anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a future PR.
|
|
||
| case IngestEventQueue.pop_pending(key, n) do | ||
| {:error, :not_initialized} -> | ||
| Logger.warning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Logger.warning( | |
| Logger.error( |
| alias Logflare.Backends.Adaptor | ||
| alias Logflare.Backends.ConsolidatedSup | ||
|
|
||
| @default_interval 30_000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can consider moving the reconciliation logic to a stateless periodic job (hourly?) using quantum, since this 30s is high for the moduledoc goals (bullet point 2), as pt2 should happen very infrequently.
pt1 should already be handled on ingestion, a 30s wait would be too slow anyway.
| def start_pipeline(%Backend{} = backend) do | ||
| adaptor_module = Adaptor.get_adaptor(backend) | ||
| child_spec = adaptor_module.child_spec(backend) | ||
| DynamicSupervisor.start_child(@dynamic_sup_name, child_spec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: will probably need to partition this in future to prevent blocking, since DynamicSupervisor can be a bottleneck.
| ) | ||
| end | ||
|
|
||
| @spec build_telemetry_metadata(state :: DynamicPipeline.state()) :: %{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine for now, we can make this module more agnostic (and also easier to open source in the future) by hoisting the telemetry metadata building and/or callback definition to the caller.
| Logger.warning("Failed to start consolidated pipeline", | ||
| backend_id: backend.id, | ||
| reason: inspect(reason) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Logger.warning("Failed to start consolidated pipeline", | |
| backend_id: backend.id, | |
| reason: inspect(reason) | |
| ) | |
| Logger.warning("Failed to start consolidated pipeline: #{inspect(reason)}", | |
| backend_id: backend.id | |
| ) |
| if Adaptor.consolidated_ingest?(backend) do | ||
| case ConsolidatedSup.stop_pipeline(backend) do | ||
| :ok -> | ||
| Logger.info("Stopped consolidated pipeline", backend_id: backend.id) | ||
|
|
||
| {:error, :not_found} -> | ||
| :ok | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be done with a with/do
Split from #3084
See #3090 before merging this
BufferProducerNote that this is based off the branch in #3083 - so it looks a bit larger than it really is.