-
Notifications
You must be signed in to change notification settings - Fork 8
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
Prune data #36
Prune data #36
Conversation
|
||
for (const beacon of beaconsToPrune) { | ||
const { airnode, templateId } = beacon; | ||
const allSignedDatas = signedDataCache[airnode]![templateId]!; // Assume the data is inserted the cache. |
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.
"data" is already the plural form 😛 "datum" is the singular
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.
Yeah, but we always use data
in singular and then there is the dillemma how to call a "group" of data. After a bit of internal pain, the "datas" was the best I could come up with. Calling it allSignedDataArray
was second on my internal list afair. Do you like more?
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.
I guess it's fine as is. Not sure how I feel about the alternative
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.
I also find datas
useful (specifically calldatas
in a multicall situation) but then https://github.com/api3dao/manager-multisig/pull/213#discussion_r1284344567
// Removes all signed data that is no longer needed to be kept in memory (because it is too old and there exist a newer | ||
// signed data for each endpoint). The function is intended to be called after each insertion of new signed data for | ||
// performance reasons, because it only looks to prune the data that for beacons that have been just inserted. | ||
export const prune = async (signedDataArray: SignedData[], maxIgnoreAfterTimestamp: number) => { |
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.
Should the timestamp include the unit in the name? Or should it rather be a Date
and the function handles converting to a Unix timestamp internally?
It's not obvious to me if "maxIgnoreAfterTimestamp" is in seconds or milliseconds and requires me to dig through several other functions to figure this out.
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.
Timestamp should imo always refer to unix timestamp and thus be in seconds. We also use this implicitly for signed data currently (e.g. the input for the signed API expects the timestamp
signed-api/packages/api/src/schema.ts
Line 45 in 044a9d7
timestamp: z.string(), |
Also, my intention is for the cache to work with timestamps and not delays. That's why I made API handlers convert the required delay to timestamp before calling the cache functions.
ba105d7
to
0ada41d
Compare
Closes #35
Rationale
I implemented it as a step during insert for performance and easy-ness to implement. The pruning function takes a list of signed data for which it tries to remove no longer needed data. The pruning function is implemented as a separate function to reduce coupling between insertion and removal of the data.