Skip to content

integration-tests: make ITParallelIndexTest still work in parallel#7211

Merged
gianm merged 2 commits intoapache:masterfrom
apollographql:glasser/it-parallel-stay-parallel
Mar 9, 2019
Merged

integration-tests: make ITParallelIndexTest still work in parallel#7211
gianm merged 2 commits intoapache:masterfrom
apollographql:glasser/it-parallel-stay-parallel

Conversation

@glasser
Copy link
Copy Markdown
Contributor

@glasser glasser commented Mar 8, 2019

Follow-up to #7181, which made the default behavior for index_parallel tasks
non-parallel.

@jihoonson
Copy link
Copy Markdown
Contributor

+1 after CI. Thank you @glasser!

@glasser
Copy link
Copy Markdown
Contributor Author

glasser commented Mar 8, 2019

Though I wonder if the tests should check that the expected number of sub tasks were created.

@glasser
Copy link
Copy Markdown
Contributor Author

glasser commented Mar 8, 2019

Also I should put the field on the right object.

@jihoonson
Copy link
Copy Markdown
Contributor

@glasser oops. Thank you for checking the spec again. I think checking number of created tasks should be covered by unit tests.

@jihoonson
Copy link
Copy Markdown
Contributor

Maybe it’s not covered yet. If so, we may need to improve ParallelIndexSupervisorResourceTest to cover it.

Follow-up to #7181, which made the default behavior for index_parallel tasks
non-parallel.
@glasser
Copy link
Copy Markdown
Contributor Author

glasser commented Mar 8, 2019

OK, this is passing for me now.

The first commit just adds the missing task spec lines as requested.

The second commit validates that at least one sub task got run per indexing run. If you don't think that's necessary, feel free to drop it from the branch before merging, but it seems nice to validate the issue this PR is fixing.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

Ok. The second commit sounds good to me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The error message should be more detailed. How about The supervisor task[taskId] didn't create any sub tasks. Was it executed in the parallel mode?

@glasser
Copy link
Copy Markdown
Contributor Author

glasser commented Mar 8, 2019

@jihoonson done. (and moved it into a different function, which makes it accessible to doReindexTest as well, which #7048 uses)

@glasser
Copy link
Copy Markdown
Contributor Author

glasser commented Mar 8, 2019

CI passed.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

@glasser thanks, LGTM!

@gianm gianm merged commit de55905 into apache:master Mar 9, 2019
@jihoonson jihoonson added this to the 0.15.0 milestone May 16, 2019
@pcarrier pcarrier deleted the glasser/it-parallel-stay-parallel branch June 7, 2019 19:15
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