-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Retry Tracking and Permissions for Tables in Hasura #663
Conversation
Manual testing performed:
I ran all with multiple blocks to confirm caching behavior works correctly. None ran more than twice. |
return; | ||
} | ||
const metadataTable = '__metadata'; | ||
async getTrackedTablesWithPermissions (indexerConfig: IndexerConfig): Promise<TrackedTablePermissions> { |
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.
Transforms the returned list into a map to make some checks a bit easier.
41f7af9
to
0620567
Compare
await this.trackTables(indexerConfig.schemaName(), needsTrackingTables, indexerConfig.databaseName()); | ||
} | ||
if (needsPermissionsTables.length > 0) { | ||
await this.addPermissionsToTables(indexerConfig, needsPermissionsTables, permissionsToAdd); |
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.
Will this fail if a table has some of the permissions, i.e. just select
/insert
, and not the others? This will attempt to set them again?
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 did not cover partial failures of adding permissions. The current code just expects it to be a all or none. I did configure the code to accept different lists of permissions (even though we currently set all of them). I felt like that was something we can potentially cover in the future rather than now.
d0f7fb2
to
2cb8da0
Compare
15d4b4a
to
3907fef
Compare
await this.deps.provisioner.provisionLogsIfNeeded(this.indexerConfig); | ||
await this.deps.provisioner.provisionMetadataIfNeeded(this.indexerConfig); | ||
await this.deps.provisioner.provisionLogsAndMetadataIfNeeded(this.indexerConfig); | ||
await this.deps.provisioner.ensureConsistentHasuraState(this.indexerConfig); |
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.
Eventually I think we should put this within provisionUserApi()
and have it retry, but is probably fine living here for now
Logs Table and Metadata Table are necessary for important functions of QueryApi. Hasura often invisibly fails to track tables and add permissions. These operations need to be successful, so I added a check which verifies tracking and permissions are correct, and reattempts them if not. When successful, the result is cached. In a successful case, the expensive hasura calls (getTableNames and getTrackedTablesWithPermissions) are done at most twice.
I also combined the conditional provisioning functions since they are verified to work already.