Addresses multiple changes #198
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The consumer auth header name and user agent for calling the consumer was not being used. This change fixes that. This commit fixes newscred#173
MunifTanjim
reviewed
Dec 22, 2024
| @@ -128,7 +129,8 @@ var callConsumer = func(httpClient *http.Client, requestID string, logger zerolo | |||
| req.Header.Set(headerBrokerPriority, strconv.Itoa(int(job.Priority))) | |||
| req.Header.Set(headerChannelID, job.Data.Message.BroadcastedTo.ChannelID) | |||
| req.Header.Set(headerConsumerID, job.Data.Listener.ConsumerID) | |||
| req.Header.Set(headerConsumerToken, job.Data.Listener.Token) | |||
| req.Header.Set(worker.consumerConnectionConfig.GetTokenRequestHeaderName(), job.Data.Listener.Token) | |||
| req.Header.Set(headerUserAgent, worker.consumerConnectionConfig.GetUserAgent()) | |||
There was a problem hiding this comment.
Should we add a hard-coded prefix (webhook-broker vX.X.X) to the user supplied user-agent value?
There was a problem hiding this comment.
Not sure we want to do it, I do not see any specific value proposition for this. We could add it to the default user agent though; that would be a separate ticket.
The notable changes are - 1. Adding a status to count model for repo to return 2. Repo function for Message status to count by channel 3. Repo function for job status to count per consumer per channel
This will show us status of messages dispatched vs not; also added filtering in the messages endpoint by status. This change addresses message part of newscred#153
In doing so also fix the dependency chain to add the endpoint to regular endpoints. This is also a part of newscred#153
A blank post would be suffice to requeue a dead job. The reason for adding a single job requeue is, if there is a consumer error and we want to test whether consumer is fixed, for push consumers, this endpoint will act like "step into" for testing a single job before requeue-ing the entire DLQ. Plus this could be used for more fine grain control on error recovery. This is also part of resolution for newscred#153
This endpoint allows us to see which consumers in which channels have what jobs in what states. Especially useful as the single endpoint to observe for dead jobs or how many jobs are inflight etc. This is done for newscred#153
This will allow for metrics to paint a "age' picture. This is done for newscred#190 Also increased timeout for unit tests to 45s as storage tests are taking more than 30s locally.
In addition to querying them to put them inflight, there is a second job of marking a pull job dead in case of the consumer reaching retry limit. So the query does return pull jobs in case they have reached the retry threshold to mark them dead. This resolves newscred#199
This endpoint would be particularly useful for testing, requeueing job's when there is an error. So the log message has the necessary IDs so that we can look them up for using the endpoint. This resolves newscred#111
imyousuf
force-pushed
the
misc
branch
2 times, most recently
from
4cc4a0b to
5b62a94
Compare
The setup is, there is a new container that is based of the webhook broker container, but also runs a nginx to expose the jsonl files written to it. The integration test first modifies the DB records directly to ensure the messages can be pruned and then tests whether at least a single file is written with jsonl. This ensures the basic pruning flow is working. This is for newscred#154
MunifTanjim
reviewed
Jan 6, 2025
controllers/job.go
Outdated
| @@ -19,6 +19,7 @@ const ( | |||
| jobsPath = consumerPath + "/queued-jobs" | |||
| jobIDPathParamKey = "jobId" | |||
| jobPath = consumerPath + "/job/:" + jobIDPathParamKey | |||
| jobRequeuePath = consumerPath + "/job/:" + jobIDPathParamKey + "/requeue-dead-job" | |||
There was a problem hiding this comment.
just a note... requeue-dead-job sounds kinda unnecessarily verbose. maybe requeue should convey the same message?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a PR that contains one independent change in every commit. They are listed below