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

JAMES-2586 Fix some jmap postgres integration tests #2048

Conversation

hungphan227
Copy link
Contributor

No description provided.

@quantranhong1999
Copy link
Contributor

org.apache.james.events.PostgresEventDeadLettersTest

Related?

@Arsnael
Copy link
Contributor

Arsnael commented Mar 4, 2024

Please rebase

@hungphan227 hungphan227 force-pushed the JAMES-2586-Fix-some-JMAP-postgres-integration-tests branch 2 times, most recently from ec122f5 to 857c591 Compare March 4, 2024 10:01
}
return Mono.just(messageBuilder.build());
}, ReactorUtils.DEFAULT_CONCURRENCY);
}

private Mono<SimpleMailboxMessage> retrieveFullMessage(SimpleMailboxMessage.Builder messageBuilder, Record record) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see a lot of methods were written for adding attachment metadata to SimpleMailboxMessage.Builder.
it is similar to what we did in AttachmentLoader

Can we reuse it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember that AttachmentLoader's method does not fit this case

Copy link
Contributor

Choose a reason for hiding this comment

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

let me try it a bit

Copy link
Contributor

Choose a reason for hiding this comment

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

caught

  • Current AttachmentLoader get each by each MessageAttachmentMetadata by id
  • Your new code: get list MessageAttachmentMetadata by list id 👍

I have a refactor for that 437550f
can you look it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great. tks. could you push it to this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have permission, you can fetch my code, and cherry-pick this commit

Copy link
Contributor

@Arsnael Arsnael Mar 8, 2024

Choose a reason for hiding this comment

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

FYI that commit is what is making tests failing/hanging forever on imap mpt tests

@vttranlina
Copy link
Contributor

Related to the ci failed

bodyStructureShouldSupportSpecificHeaders(GuiceJamesServer) – org.apache.james.jmap.rfc8621.postgres.PostgresEmailGetMethodTest

JSON documents are different:
Different value found in node "methodResponses[0][1].list[0].bodyStructure.blobId", expected: <"1_1"> but was: <"018e11c3-8f00-7d96-a046-4f8beef18834_1">.

I see we get the same in the Distributed test (master branch)
image

It looks like comes from test cases, not related to Postgres or not
//
I'm investigating it

@vttranlina
Copy link
Contributor

Related to the ci failed bodyStructureShouldSupportSpecificHeaders(GuiceJamesServer)

we can cherry-pick fixup commit: #2093

@hungphan227 hungphan227 force-pushed the JAMES-2586-Fix-some-JMAP-postgres-integration-tests branch from 857c591 to 5a716f0 Compare March 6, 2024 07:58
@Arsnael
Copy link
Contributor

Arsnael commented Mar 7, 2024

03:25:25.251 [INFO ] o.a.j.b.p.DockerPostgresSingleton - 2024-03-07 03:25:25.250 UTC [62] LOG:  checkpoint starting: time
03:27:57.599 [INFO ] o.a.j.b.p.DockerPostgresSingleton - 2024-03-07 03:27:57.598 UTC [62] LOG:  checkpoint complete: wrote 1524 buffers (9.3%); 0 WAL file(s) added, 0 removed, 1 recycled; write=152.270 s, sync=0.001 s, total=152.348 s; sync files=0, longest=0.000 s, average=0.000 s; distance=16207 kB, estimate=16207 kB; lsn=0/28E6CE0, redo lsn=0/28E6CA8

Hanging again, same point... do we have an issue here in our pg test with the docker postgres signleton extension?

we should investigate...

I'm suspecting something being potentially unstable on our side as all the timeout builds have occured on different workers in the last few days

@quantranhong1999
Copy link
Contributor

I'm suspecting something being potentially unstable on our side as all the timeout builds have occured on different workers in the last few days

Any chance it is related to this change:
https://github.com/apache/james-project/pull/2072/files

...

@Arsnael
Copy link
Contributor

Arsnael commented Mar 7, 2024

No I don't think so... I think I have a clue, and maybe we are a bit unlucky on some of our builds... let me try something and open an other PR, I noticed a pattern when such thing happens

@Arsnael
Copy link
Contributor

Arsnael commented Mar 7, 2024

@quantranhong1999 => #2095

My 50 cents, let's see :)

@Arsnael
Copy link
Contributor

Arsnael commented Mar 8, 2024

@hungphan227 @vttranlina I'm going on my PR to test back without the last commit, could not fix it.

Feel free to fix it yourselves if you want it back

For example locally you can try to run UidSearchOnIndex, you will see quickly it's hanging forever

@vttranlina
Copy link
Contributor

fixup! JAMES-2586 Optimize AttachmentLoader

Fixed 144ac90

Reason: Hanging when Collections is empty

@Arsnael
Copy link
Contributor

Arsnael commented Mar 11, 2024

Thanks.

Can you test guys locally on those tests and make sure it passes with those fixes? => https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2095/3/testReport/

@vttranlina
Copy link
Contributor

Thanks.

Can you test guys locally on those tests and make sure it passes with those fixes? => https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2095/3/testReport/

  • webSocketShouldPushNewMessageWhenChangeSubscriptionOfMailbox -> failed on my local. Related it, I discussed it with @hungphan227 on last Thursday, it failed because the test suite code, was not related to Postgres -> should update the test code

  • checkShouldTurnFromDegradedToHealthyAfterMoreReadsOnAMissedProjection -> failed

  • shouldFailWithCannotCalculateChangesWhenSingleChangeIsTooLarge -> passed

@hungphan227 hungphan227 force-pushed the JAMES-2586-Fix-some-JMAP-postgres-integration-tests branch from ac1309f to b2ead22 Compare March 11, 2024 07:13
@Arsnael
Copy link
Contributor

Arsnael commented Mar 12, 2024

11:29:37,710 [INFO] Apache James :: Server :: JMAP RFC-8621 :: Postgres Integration Testing FAILURE [46:53 min]

46 mins... Maybe unstable CI or maybe not. Will restart the build to see

@Arsnael Arsnael force-pushed the JAMES-2586-Fix-some-JMAP-postgres-integration-tests branch from b2ead22 to ec60c86 Compare March 12, 2024 07:16
@Arsnael
Copy link
Contributor

Arsnael commented Mar 12, 2024

Build was green (finally)

I squashed and rebased

@Arsnael
Copy link
Contributor

Arsnael commented Mar 13, 2024

The JMAP integration test part seems to take often 40 ~ 50 minutes to complete which is too long (we consider less than 30 minutes ok)

@hungphan227 Can we investigate again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants