fix: consistent naming for resolving payloads #90
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: backend/lib/piece-indexer-service.js
Did you find this useful? React with a 👍 or 👎 |
| export const lookUpPayloadCidsLoop = async (makeRpcRequest, getDealPayloadCid, pgPool) => { | ||
| const LOOP_NAME = 'Look up payload CIDs' | ||
| export const resolvePayloadCidsLoop = async (makeRpcRequest, resolvePayloadCid, pgPool) => { | ||
| const LOOP_NAME = 'Resolve payload CIDs' |
There was a problem hiding this comment.
We will have to change the Grafana query after this change.
| const numOfUnresolvedPayloadCids = await countStoredActiveDealsWithUnresolvedPayloadCid(pgPool) | ||
| if (INFLUXDB_TOKEN) { | ||
| recordTelemetry('look_up_payload_cids_stats', point => { | ||
| recordTelemetry('resolve_payload_cids_stats', point => { |
There was a problem hiding this comment.
Will need to change the Grafana query after this change
| import { findAndSubmitUnsubmittedDeals, submitDealsToSparkApi } from '../lib/spark-api-submit-deals.js' | ||
| import { getDealPayloadCid } from '../lib/piece-indexer-service.js' | ||
| import { resolvePayloadCid } from '../lib/piece-indexer-service.js' |
There was a problem hiding this comment.
This is confusing: One function is called resolvePayloadCids, the other resolvePayloadCid. Looking at the name, I would expect the 1st to do the same as the 2nd, only for multiple payload CIDs.
It's different from that though: The 1st looks up what to resolve, calls the look up function, and also saves the state in the DB.
I don't have a solution on the top of my head. @bajtos wdyt?
There was a problem hiding this comment.
I think we're using "to resolve" for two things here:
- Content resolution (similar to name resolution)
- Marking a task as resolved
We should use different terms for these.
There was a problem hiding this comment.
@NikolasHaimerl do you have a suggestion? Let's use "resolve" for one of the two cases from the previous comment, and another term for the other
There was a problem hiding this comment.
You are suggesting we use two different verbs, one for the action of fetching the payloads via HTTP and another for the overall process of filling payload CIDs in the database that are currently set to NULL. Did I understand that correctly?
There was a problem hiding this comment.
What I'm saying is that this is currently inconsistent naming. Consistent naming means to give the same thing the same name, but in turn, to give different things different names.
Here we have two processes (currently called resolvePayloadCid and resolvePayloadCids), that are different (in more ways than their multitude). Therefore at least one of them needs a different name.
We want consistent naming for resolving payload CIDs:
The action is
resolvingthe state isresolvedorunresolved.Closes #87