Skip to content
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: Cache Database Credentials #585

Merged
merged 5 commits into from
Feb 28, 2024
Merged

feat: Cache Database Credentials #585

merged 5 commits into from
Feb 28, 2024

Conversation

darunrs
Copy link
Collaborator

@darunrs darunrs commented Feb 27, 2024

Currently, Runner fetches DB credentials every invocation of runFunction because provisioning of the user's DB occurs in Runner. As such, we couldn't be sure if we could fetch credentials for the user's DB from Hasura or not. The fetch is slow and increases the latency of any indexers which leverage context.db.

I updated the code to cache the credentials locally after the first successful fetch. This allows all future requests to succeed as these credentials will never change after they are set. The changes were aimed to be minimal as we intend to migrate provisioning out of Runner. In fact, the changes aid in the effort as the credentials fetch is done through the Provisioner class rather than separately in DmlHandler.

@darunrs darunrs requested a review from a team as a code owner February 27, 2024 01:41
grpcServer.forceShutdown();
}
})();
void (async function main () {})();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went ahead and cleaned up the Redis scanning here too. I can also move this to happen in another PR, if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this function anymore

let currBlockHeight = 0;
let indexerName = `${workerContext.indexerConfig.account_id}/${workerContext.indexerConfig.function_name}`;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migrated to using config passed into worker, since we no longer use storage after V2 migration.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update StreamHandler too please? I think it still assumes indexerConfig can be undefined

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes you're totally right. I'll update that!

@darunrs darunrs linked an issue Feb 27, 2024 that may be closed by this pull request
grpcServer.forceShutdown();
}
})();
void (async function main () {})();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this function anymore

export default class Indexer {
DEFAULT_HASURA_ROLE;
database_connection_parameters: any | undefined = undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid using any please

// Cache database credentials after provisioning
try {
this.database_connection_parameters = this.database_connection_parameters ??
await this.deps.provisioner.getDatabaseConnectionParameters(functionName.split('/')[0].replace(/[.-]/g, '_'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await this.deps.provisioner.getDatabaseConnectionParameters(functionName.split('/')[0].replace(/[.-]/g, '_'));
await this.deps.provisioner.getDatabaseConnectionParameters(hasuraRoleName);

This is already defined above

let currBlockHeight = 0;
let indexerName = `${workerContext.indexerConfig.account_id}/${workerContext.indexerConfig.function_name}`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update StreamHandler too please? I think it still assumes indexerConfig can be undefined

@darunrs darunrs merged commit c6ed811 into main Feb 28, 2024
3 checks passed
@darunrs darunrs deleted the storeHasuraCreds branch February 28, 2024 18:08
@darunrs darunrs mentioned this pull request Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store Hasura Credentials Fetch
2 participants