Concurrent file uploads and result creation #34
Conversation
hi-rai
requested review from
AndrianBdn,
ramilamparo,
satvik007 and
sirkostya009
Summary of ChangesHello @hi-rai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant performance enhancements to the result submission process by enabling concurrent file uploads and batch creation of test results. It refactors the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces concurrent file uploads and result creation, addressing issue #33. It includes changes to the package.json version, updates to API schemas and run APIs, modifications to the result uploader for handling attachments and batch result creation, and adjustments to the twirl loader in misc.ts. The most significant changes involve refactoring the uploadTestCases function in ResultUploader.ts to handle attachment uploads and result creation in batches, improving efficiency and concurrency.
| for (const item of items) { | ||
| const promise = handler(item) | ||
| .then((result) => { | ||
| results.push(result) | ||
| }) | ||
| .finally(() => { | ||
| executing.delete(wrappedPromise) | ||
| }) |
There was a problem hiding this comment.
The processConcurrently function uses a Set to track executing promises and Promise.race to limit concurrency. However, the finally block only deletes the wrappedPromise from the executing set. If the handler function throws an error, the wrappedPromise will be rejected, but the original promise will still be in the executing set, potentially leading to incorrect concurrency control.
To fix this, ensure that the original promise is deleted from the executing set in the finally block.
.finally(() => {
executing.delete(promise)
})
satvik007
left a comment
satvik007
left a comment
There was a problem hiding this comment.
Test it once when new production deployment happens.
| import { ResultUploadCommandArgs, UploadCommandType } from './ResultUploadCommandHandler' | ||
|
|
||
| const MAX_CONCURRENT_FILE_UPLOADS = 10 | ||
| let MAX_RESULTS_IN_REQUEST = 50 // Only updated from tests, otherwise it's a constant |
There was a problem hiding this comment.
Its probably not that bad, but MAX_RESULTS_IN_REQUEST should really be a constant and there could be a factory style way to create ResultUploader where it could be modified. The simplest way to be to just not have those test cases that rely on this, or fix it.
There was a problem hiding this comment.
The simplest way to be to just not have those test cases that rely on this, or fix it - The tests don't rely on it, but rather verify the expected behavior by modifying the value
Since in tests we directly run the command (which in turn creates ResultUploader), instead of directly creating ResultUploader, the only way I can think of is to have a new command line argument for this. But that would make it more complex
This reverts commit 2ba5c2e.
satvik007
deleted the
dev/himanshu/concurrent-file-upload-and-result-creation
branch
This fixes #33 (comment)
IMPORTANT - This also requires create batch result public API PR to be merged in the
tmsrepo and then wait for the next release (not adding release tag for now)