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

perf(email): make fetchEmails async #726

Closed
wants to merge 6 commits into from
Closed

Conversation

donno2048
Copy link

@chuang8511
Copy link
Member

chuang8511 commented Oct 16, 2024

@donno2048
Thanks for your contribution. Please fix the bugs and make PR open again!

Summary

Please fix the "1." & "2." cases.

Not as Expected Cases

The case with search case is not as expected.

  1. The order is changed. We expected to fetch the email from latest email. But, it is from the earliest email.
    Screenshot 2024-10-16 at 2 53 46 PM

  2. Some content disappeared
    Screenshot 2024-10-16 at 2 53 39 PM

Good Cases

The case without search setting works fine, and the performance is good!

The end to end test with search limit: 100
originally
"M":"TriggerPipelineWorkflow completed in","duration":"34.519138682s"

After this PR merged
"M":"TriggerPipelineWorkflow completed in","duration":"27.063491595s"

image

Sample Recipe

version: v1beta
component:
    email-0:
        type: email
        task: TASK_READ_EMAILS
        input:
            search:
                limit: 100
                mailbox: Inbox
        setup:
            email-address: ${variable.your_gmail_address}
            password: ${secret.email}
            server-address: imap.gmail.com
            server-port: 993
variable:
    your_gmail_address:
        title: your-gmail-address
        instill-format: string
output:
    result:
        title: result
        value: ${email-0.output.emails}

@chuang8511 chuang8511 marked this pull request as draft October 16, 2024 13:57
@donno2048 donno2048 marked this pull request as ready for review October 16, 2024 20:25
@donno2048
Copy link
Author

donno2048 commented Oct 16, 2024

@chuang8511 please review.

Also, I think the performance boost will be even greater for a bigger search space than 100.

@chuang8511
Copy link
Member

chuang8511 commented Oct 21, 2024

@donno2048
I guess the output data is not set. Could you confirm it again?
image

Sample Recipe

version: v1beta
component:
    email-0:
        type: email
        task: TASK_READ_EMAILS
        input:
            search:
                limit: 5
                mailbox: Inbox
        setup:
            email-address: ${variable.your_gmail_address}
            password: ${secret.email}
            server-address: imap.gmail.com
            server-port: 993
variable:
    your_gmail_address:
        title: your-gmail-address
        instill-format: string
output:
    result:
        title: result
        value: ${email-0.output.emails}

@chuang8511
Copy link
Member

@donno2048

The order seems still wrong.

image

Would you mind to do QA before requesting review?
I will check these 3 cases as I mentioned before.

Thank you!

@donno2048
Copy link
Author

The problem is I don't have a PC and I can't really do that on mobile... Sorry...

@chuang8511
Copy link
Member

The problem is I don't have a PC and I can't really do that on mobile... Sorry...

Sorry, just out of curiosity. You always develop in mobile?
Sound crazy cool. 😆

OK, then let me QA for you. Please check your code again.

@donno2048
Copy link
Author

donno2048 commented Oct 22, 2024

The problem is I don't have a PC and I can't really do that on mobile... Sorry...

Sorry, just out of curiosity. You always develop in mobile?

@chuang8511
Yes 😅

@chuang8511
Copy link
Member

The code is not able to compile again.

Could you please find some ways to verify your code is ok?

Thank you. 🙏

@donno2048
Copy link
Author

Made a stupid mistake it'll now compile @chuang8511

@chuang8511
Copy link
Member

@donno2048
I tested it again, but the code doesn't work. 😢

  • The order is still wrong
  • When I set limit to 30, it runs forever

Please notify us to review when you find a way to test it.

The current test code is only unit testing for the unexported functions in email pkg.
If you don't have PC, probably you can write some test code to exported functions to verify your modification.

@donno2048
Copy link
Author

I don't really have a way to do this right now, closing.

Thanks anyways

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants