-
Notifications
You must be signed in to change notification settings - Fork 7
feat(agentstore): put index in dynamo #470
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
base: main
Are you sure you want to change the base?
Conversation
move the invocation index to work on top of dynamo, writing both for now
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.
🚀
task: 'string', | ||
kind: 'string', | ||
invocation: 'string', | ||
message: 'string', |
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.
Would be good to have comments explaining what these fields store.
const agentIndexBucket = new Bucket(stack, 'invocation-store', { | ||
cors: true, | ||
cdk: { | ||
bucket: getBucketConfig('invocation-store', app.stage) | ||
} | ||
}) | ||
|
||
const agentIndexTable = new Table(stack, 'invocation-table', agentIndexTableProps) |
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.
Minor, but it's an index of items in agent messages, so I'd probably go for agent-index
or agent-message-index
as the table name. Whilst invocation-table
aligns with the name of the old bucket (invocation-store
), we have long since moved on from calling this invocation-store
in the (majority of the) code. agent-index
aligns with what we're calling it in the code today.
Also, I don't think we need need to suffix with -table
- all dynamo tables are tables.
const agentIndexTable = new Table(stack, 'invocation-table', agentIndexTableProps) | |
const agentIndexTable = new Table(stack, 'agent-index', agentIndexTableProps) |
region, | ||
buckets: { | ||
message: { name: mustGetEnv('WORKFLOW_BUCKET_NAME') }, | ||
index: { name: mustGetEnv('INVOCATION_BUCKET_NAME') }, | ||
}, | ||
tables: { | ||
index: { name: mustGetEnv('INVOCATION_TABLE_NAME') }, |
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.
Hmm, should this be AGENT_INDEX_TABLE_NAME
?
TableName: dynamoDBTableName, | ||
Key: { | ||
"task": { | ||
S: encodedInvocationKeyPrefix |
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 believe these are just CID strings (i.e. no /
suffix)
S: encodedInvocationKeyPrefix | |
S: `${invocationCid}` |
TableName: connection.tables.index.name, | ||
Key: { | ||
"task": { | ||
S: prefix |
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.
To remove the trailing slash:
S: prefix | |
S: prefix.slice(0, -1) |
Goals
I'm not sure why the agent store was setup to use S3 keys as an index of invocations, but the net effect is we end up using a ListObjectsV2 operation, which is a tier 1 op (see private note additional context)
Proposed approach to resolving this issue is:
Implementation