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: add composedb correctness test that perfroms parallelization #115

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

samika98
Copy link
Contributor

Composedb test for investigating gotcoin failures

Copy link

@gvelez17 gvelez17 left a comment

Choose a reason for hiding this comment

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

lets do it!

Comment on lines +7 to +12

export const MultiQuerySchema = `
type MultiQuerySchema @createModel(accountRelation: LIST, description: "A set of unique numbers")
@createIndex(fields: [{ path: "myData" }]){
myData: String! @string(maxLength: 500)
}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be the same schema as the BasicSchema above, any reason to duplicate please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we want to plug in the schema that gitcoin uses or something similar. This is a TODO


const queryResponses = await Promise.all(queryPromises)
queryResponses.forEach((queryResponse) => {
const queryResponseObj = JSON.parse(JSON.stringify(queryResponse))
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of the stringify+parse here please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

create a deep copy of the queryResponse

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it need a deep copy of the data here please?

Comment on lines +188 to +192
for (let i = 0; i < parallelIterations; i++) {
queryPromises.push(
composeClient3.executeQuery(getDocumentsByStreamIdQuery, getDocumentsByStreamIdVariables),
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expected behavior here please?
Seems like this is going to execute 20 queries using the 20 IDs of the created documents? It's likely the batching logic of the ComposeDB runtime will dedupe the calls though, so in effect it should only perform a single multiquery of 20 documents.
If the goal is to create load on the node, better use the Ceramic APIs directly I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to perform 20 multi-query operations simultaneously. So the query of getDocumentsByStreamId will have to change.


test('Create 20 model instance documents in a second and query them from the same node using mult query in the same node, query 20 times ina second', async () => {
for (let i = 0; i < transactionIterations; i++) {
// TODO : Add whatever mutation gitcoin was doing
Copy link
Contributor Author

@samika98 samika98 Apr 24, 2024

Choose a reason for hiding this comment

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

@PaulLeCam We want to plug in the mutation which will enable us to perform a multi-query here

const documentIds = await Promise.all(createDocumentPromises)
documentIds.forEach((id) => expect(id).toBeDefined())

// TODO: Create a multi query instead of simple query
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PaulLeCam We want to create a graphql query which perfoems multi-query

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.

3 participants