Repository navigation
Add memento adaptor - #1350
Add memento adaptor#1350
memento adaptor#1350Conversation
josephjclark
left a comment
There was a problem hiding this comment.
Some comments and observations. I think the tests need some more work.
One thing I'm slightly concerned with in the auto throttler is that it takes a very narrow view.
Presumably throttling is done based on the IP address. Or maybe API key? So we potentially have a problem if you're running two workflows concurrently - perhaps even on different projects. The rate limiter in the adaptor only knows about a single job's rate - it can't see what other clients are calling within its shared limit.
I suppose that's an argument for being a bit dumber: keep requesting until you get a rate limit, then sleep for n seconds and try again.
It may also be an argument for a larger default page size. Larger pages means fewer requests, and as 10 requests per minute feels quite low to me, we should probably lean in that direction
|
@josephjclark i have worked on your feedback, Can you give this another round of review ? |
josephjclark
left a comment
There was a problem hiding this comment.
Looks pretty good Mtuchi but apart from some nitpicks, my concern is that I don't have a lot of confidence, from the implementation or the tests, that this is actually working as intended.
So I'm worried a) that if we release this it doesn't really work, and b) the next person to develop it won't be working on solid ground.
I don't know how much more time we want to spend on this, we could just release. But I'm a little nervous tbh
cec1acc to
77bce10
Compare
josephjclark
left a comment
There was a problem hiding this comment.
Almost happy! Just a couple of questions and comments in tests, but I think we can get this merged soon
| @@ -0,0 +1,66 @@ | |||
| export const mockEntriesPagination = (testServer, path, opts = {}) => { | |||
There was a problem hiding this comment.
Basically at the moment you're setting up a mock interceptor for every page required. It's quite hard to configure.
What if you set up a single interceptor which does real pagination?
This is pseudocode really but something like this:
const createPaginatingEndpoint = (path, data) => {
// Set up a totally generic interceptor at the path
testServer
.intercept({
path,
method: 'GET',
})
.reply((req) => {
// I can't remember this syntax
const { pageSize, token } = req.params;
return getPaginatedResponse(data, token, pageSize)
})
}
const getPaginatedResponse = (data, token, pageSize) => {
const entries = data.slice(token, pageSize)
const nextPageToken = token + pageSize < data.length ? token + pageSize : undefined
return {
entries,
nextPageToken,
revision: Math.floor(Math.random() * 100),
}
}
We use this same pattern in kobotoolbox and something similar in openmrs. Once you've written that paginated response generator once, you can re-use it on different datasets and you'll get totally natural responses
|
@mtuchi Two small things and then I'm happy to merge this down - let's get this done today. Pnpm lock also needs updating |
|
@josephjclark i have addressed your feedback and |
Summary
Add a new memento adaptor functions and http functions for custom implementation
Fixes #1336
Details
autoThrottleinutil.requestWithPaginationfunctionhandleRateLimitandhandleRateLimitExceededutils for helping with auto throttleAI Usage
Please disclose how you've used AI in this work (it's cool, we just want to
know!):
You can read more details in our
Responsible AI Policy
Review Checklist
Before merging, the reviewer should check the following items:
production? Is it safe to release?
dev only changes don't need a changeset.