New metadata structure/trigger worker#4816
Conversation
| // It retries on CouchDB conflict errors (409) up to maxRetries times, | ||
| // re-fetching a fresh document revision each iteration to avoid stale-rev | ||
| // conflicts. Both the boolean flag and the timestamp are written in a single | ||
| // UpdateDoc call to stay atomic. |
There was a problem hiding this comment.
Please don't describe what the implem does except for non-trivial stuff. Implementation can change over time, and comments like this are easy to forget
| if indexed { | ||
| newdoc.CozyMetadata.RAG.LastSuccessDate = &now | ||
| } else { | ||
| newdoc.CozyMetadata.RAG.LastErrorDate = &now |
There was a problem hiding this comment.
There is a confusion here: indexed: false is not an error, we can have this value when we delete a file. So for me, those LastSuccessDate / LastErrorDate should be provided in the webhook directly, alongside the Indexed info.
| log.Infof("Retrying RAG status update for file %s (attempt %d/%d)", file.DocID, i+1, maxRetries) | ||
| } | ||
| var f *vfs.FileDoc | ||
| f, err = fs.FileByID(file.DocID) |
There was a problem hiding this comment.
you already have the file provided as func parameter
| log := inst.Logger().WithNamespace("rag") | ||
| fs := inst.VFS() | ||
|
|
||
| for i := 0; i < maxRetries; i++ { |
There was a problem hiding this comment.
I don't think this maxRetries is useful, there is already an automatic worker retry. I would simply remove the loop. For conflicts, we could have this logic: if a conflict occurs, it means a parallel webhook was called. So, compare the current file LastSuccessDate and LastErrorDate with the received dates. The most recent date win: if it's the ones from file: drop it, no need to update the doc. If it's the new dates: do a new fetch to have the latest file and update
| log.Debugf("RAG webhook trigger already exists: %s", t.ID()) | ||
| return webhookURL, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we can do better than that : currently, it means we fetch all the triggers from db, and check if the correct trigger exist. It's ok, but not very efficient. We could do that with one db query.
When you create the trigger, provide a TID: https://github.com/cozy/cozy-stack/blob/master/model/job/scheduler.go#L63 ; for instance TID=RAG_STATUS_TRIGGER_NAME, with RAG_STATUS_TRIGGER_NAME="rag-index-status"
Then, you can query it directly with a sched.GetTrigger(inst, RAG_TRIGGER_STATUS_ID)
ewan102
force-pushed
the
New-MetadataStructure/trigger-worker
branch
from
30c3cb4 to
abc9df2
Compare
| } | ||
| if fcm.RAG != nil { | ||
| rag := map[string]interface{}{ | ||
| "indexed": fcm.RAG.Indexed, |
There was a problem hiding this comment.
Why is indexed not treated like the other attributes?
| FileID string `json:"file_id"` | ||
| Status string `json:"status"` // "success" | "error" | "notsupported" | ||
| Md5Sum string `json:"md5sum"` // content hash (hex) the status refers to; echoed by rag-indexer | ||
| Timestamp string `json:"timestamp"` // RFC3339Nano; absent in older emitters |
There was a problem hiding this comment.
what is "older"? I don't think we should have time-relative comment
| Partition string `json:"partition"` | ||
| FileID string `json:"file_id"` | ||
| Status string `json:"status"` // "success" | "error" | "notsupported" | ||
| Md5Sum string `json:"md5sum"` // content hash (hex) the status refers to; echoed by rag-indexer |
There was a problem hiding this comment.
"echoed by rag-indexer" -> names can change , and we could have other emitters than rag-indexer, so no need to mention it
| @@ -0,0 +1,80 @@ | |||
| package rag | |||
There was a problem hiding this comment.
we might rename this file "callback_status", this way the purpose is clearer
| // Callback about an outdated version (a newer content is now on the file): | ||
| // it must not influence Status, but a past success still means a version | ||
| // was indexed. | ||
| if isOutdated(file, md5sum) { |
There was a problem hiding this comment.
if the md5 is outdated, I think we actually should drop it entirely. setting indexed:true could actually be false, since the newest status could be indexed: false if we removed the file
ewan102
force-pushed
the
New-MetadataStructure/trigger-worker
branch
from
6953843 to
40c796e
Compare
2 participants
Adding a RAG indexation result into a typed FilesCozyMetadata.RAG sub-object. The webhook result is a boolean.
This PR changes nothing for the rest of Cozy while CallRAGindexer is not modified
and timestamp atomically in one couchdb.UpdateDoc with a 3-retry conflict loop.