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

Aws sqs test fix #3170

Merged
merged 2 commits into from Oct 20, 2021
Merged

Aws sqs test fix #3170

merged 2 commits into from Oct 20, 2021

Conversation

VratislavHais
Copy link
Contributor

This PR fixes the problem with sqsDeleteMessage test. Unfortunately when testing I have discovered that test for delay queue is broken. I am still investigating it. It looks like the problem is on AWS side as everything works as expected in CQ a camel.

@VratislavHais VratislavHais force-pushed the aws-sqs-fix branch 2 times, most recently from 8a2a299 to b396e5f Compare October 8, 2021 11:33
@zbendhiba
Copy link
Contributor

zbendhiba commented Oct 8, 2021

We can open a separate issue to fix the queue delay. So that we could fix later

@aldettinger
Copy link
Contributor

Beyond previous comments from Zineb, I think that the aggregated changes looks good.

However, I wonder whether those commits should be squashed ? For instance one commit is adding method "deleteQueueAndListQueues" and then another commit is deleting this very same method.

Also the CI build is failing, maybe running mvn clean process-resources -P format would help.

@VratislavHais
Copy link
Contributor Author

Beyond previous comments from Zineb, I think that the aggregated changes looks good.

However, I wonder whether those commits should be squashed ? For instance one commit is adding method "deleteQueueAndListQueues" and then another commit is deleting this very same method.

Also the CI build is failing, maybe running mvn clean process-resources -P format would help.

oh, you are right. This should not have happened. Not sure what got wrong with my fork

@VratislavHais VratislavHais force-pushed the aws-sqs-fix branch 2 times, most recently from 5493555 to a11bc61 Compare October 12, 2021 06:35
@VratislavHais
Copy link
Contributor Author

Are you able to reproduce the failures on your localhost? I have tried it both with localstack and with AWS credentials and the test is passing. The failing test is Aws2DdbStreamTest

@zbendhiba
Copy link
Contributor

Are you able to reproduce the failures on your localhost? I have tried it both with localstack and with AWS credentials and the test is passing. The failing test is Aws2DdbStreamTest

Is it related to this PR : #3175 ?
If yes, maybe just rebase from main

@VratislavHais VratislavHais force-pushed the aws-sqs-fix branch 2 times, most recently from f10261a to b335ed1 Compare October 12, 2021 12:27
@VratislavHais
Copy link
Contributor Author

I don't understand. It has been rebased with newest changes and yet the tests (not sqs tests) are failing...

@zbendhiba
Copy link
Contributor

I don't understand. It has been rebased with newest changes and yet the tests (not sqs tests) are failing...

Let me try this locally on my machine

@VratislavHais
Copy link
Contributor Author

@zbendhiba any luck?

@zbendhiba
Copy link
Contributor

@zbendhiba any luck?

Let me try to push differently. I feel there's a problem with the branch. Locally, the tests run just fine

@aldettinger
Copy link
Contributor

I've cherry picked this commit on top of main locally.
I've tried to run the tests isolated/grouped, with/without patch. And it runs ok in all case on my machine.
Hope this helps.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 15, 2021

@VratislavHais could you please rebase so that there is a chance to get a clean CI build?

@zbendhiba
Copy link
Contributor

@VratislavHais did rebase and it didn't work
I reset the branch to be equal to main, and added the commit on top of it, and it's still not passing the CI.
I don't know what's the problem really

@aldettinger
Copy link
Contributor

We had few successful builds on CI main those days without this PR.

I've tried to run the aws grouped tests with and without this PR locally 10 times and I ended up with:
10 successful builds without the PR
10 successful builds with the PR

So, at this stage the best explanation would be that this PR is really introducing a build issue but only on CI.
@VratislavHais: Could you please check this ? It may involve to temporarily push some experimental commits in this PR in order to understand what's happening. Unless someone else has a better idea ?

@ppalaga
Copy link
Contributor

ppalaga commented Oct 15, 2021

Thanks for the explanation @zbendhiba and @aldettinger

@VratislavHais
Copy link
Contributor Author

Hello, will do. Thank you for investigating

@VratislavHais
Copy link
Contributor Author

I really don't see how my changes can affect that. Not being able to reproduce it also does not help

@aldettinger
Copy link
Contributor

@VratislavHais If we are not sure this PR is causing the build issue. I would propose to rebase and push without the fix, have a CI build, then push the fix and have another CI build. It may be a good experiment.

If we are sure the PR is causing the issue on CI, then would it be possible to comment parts of the PRs and have intermediate CI builds to check which part would be problematic ?

If we are not able to correct, maybe implementing test in another way would be a way to avoid long investigation on CI builds ?

And yeah, you are right those issues occurring on CI only are really hard to deal with. Sadly, it happens from time to time.

@VratislavHais
Copy link
Contributor Author

I was wondering - what OS are you on @aldettinger @zbendhiba ? Maybe it can be OS related as it is running on Ubuntu? (just a wild guess)

@aldettinger
Copy link
Contributor

As far as I remember, we are running on RHEL 7 and RHEL 8 and the test passes on our machines. CI builds seem to occur on Ubuntu 20.04.3.

@VratislavHais
Copy link
Contributor Author

Thank you @aldettinger . I'll attempt to run it on Ubuntu

@aldettinger
Copy link
Contributor

@VratislavHais Well done for attempting to run the test on Ubuntu 👍 So, we now have more information that the O/S is not the root cause of this issue.

Do you have any error message in the logs by grepping something like "Failures: 1" or "Errors: 1". Maybe, it would ring any bell to someone.

@VratislavHais
Copy link
Contributor Author

@aldettinger I believe this is the problem: https://pastebin.com/xzbUFnbv
I have tried increasing the size of table for shards (as suggested by google) from 10 to 20 but it did not help. I'll continue my investigation.

@aldettinger
Copy link
Contributor

@JiriOndrusek, @VratislavHais, @zbendhiba The build is passing now, well done for this hard investigation 👍
Do you think it's valuable to merge this PR at this stage ? Or was it just to experiment the build and then we are waiting for a cleaner/long-term fix ?

@JiriOndrusek
Copy link
Contributor

JiriOndrusek commented Oct 20, 2021

@aldettinger From my POV it could be merged, even if I don't like tha fact that sqs test on CI takes ~8mins. I created an issue - #3207 - where I;d like to make timeouts for local test smaller.
But for this moment it could be probably merged. (If I understand correctly, it makes test with real AWS successful, which is a nice fix)

@JiriOndrusek
Copy link
Contributor

Fix of the aws2-ddb test, which is part of this PR, is long term fix - it fixes way how route is started. Although it is not required for ddb, it makes sense (and makes ddb test working in case that local tests are taking a lot of time - which can happen during grouped run).

Copy link
Contributor

@aldettinger aldettinger left a comment

Choose a reason for hiding this comment

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

Ok, so it's providing value and we have a ticket for further enhancements. Looks good to me.

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.

None yet

6 participants