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.

Sign up for GitHub

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

Jump to bottom

JAMES-2586 - JMAP Upload - cleanup, optimize, fix unstable test upload usage #2068

Merged
merged 3 commits into from
Mar 19, 2024
Merged

JAMES-2586 - JMAP Upload - cleanup, optimize, fix unstable test upload usage #2068

merged 3 commits into from
Mar 19, 2024

Conversation

vttranlina
Copy link
Contributor

No description provided.

@vttranlina vttranlina mentioned this pull request Feb 29, 2024
Closed
quantranhong1999
quantranhong1999 reviewed Feb 29, 2024
View reviewed changes
...r/data/data-jmap/src/test/scala/org/apache/james/jmap/api/upload/UploadServiceContract.scala
@@ -108,6 +108,7 @@ trait UploadServiceContract {
.block())

// Exceed 100 bytes limit
Thread.sleep(500)
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: any chance we can use await?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a difference.
Here I want the cleanupUploadIfNeeded completed (it was called 10 times) before triggering "exceed".

Arsnael
Arsnael reviewed Feb 29, 2024
View reviewed changes
Copy link
Contributor

@Arsnael Arsnael left a comment

Choose a reason for hiding this comment

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

Can we contribute the non postgres enhancement on jmap upload to master branch as well?

...data/data-jmap/src/test/java/org/apache/james/jmap/api/upload/UploadRepositoryContract.scala Outdated Show resolved Hide resolved
@vttranlina
Copy link
Contributor Author

The CI failed looks like not related to this pr
I tried to test it on local, the test passed
image

org.apache.james.DistributedPostgresJamesServerTest

02:47:05.340 [ERROR] t.postgres:16.1 - Could not start container
org.testcontainers.containers.ContainerLaunchException: Timed out waiting for log output matching '.*database system is ready to accept connections.*\s'
	at org.testcontainers.containers.wait.strategy.LogMessageWaitStrategy.waitUntilReady(LogMessageWaitStrategy.java:47)
	at org.testcontainers.containers.wait.strategy.AbstractWaitStrategy.waitUntilReady(AbstractWaitStrategy.java:52)
	at org.testcontainers.containers.PostgreSQLContainer.waitUntilContainerStarted(PostgreSQLContainer.java:147)

@vttranlina
Copy link
Contributor Author

Can we contribute the non postgres enhancement on jmap upload to master branch as well?

I've split it into the master branch here: #2069

As it relates to PostgreSQL changes, I'll rebase this pull request later, after the master branch has been merged.

@vttranlina
Copy link
Contributor Author

@Arsnael
Copy link
Contributor

Arsnael commented Mar 5, 2024

Why need to rebase after rebasing postgres branch with master btw?

@quantranhong1999
Copy link
Contributor

Can we merge this @vttranlina ?

@vttranlina
Copy link
Contributor Author

Can we merge this @vttranlina ?

This PR is using some code from master
I'm waiting for the task linagora#5110 done, then rebase this pr
Or I will cherry-pick a commit from the master

@vttranlina
JAMES-3925 - JMAP Upload - Method delete of Upload Repository should …
869ee61 
…return Boolean value when applied
@vttranlina
JAMES-3925 - JMAP Upload - Update current usage only when delete succeed
3e76f95
@vttranlina
JAMES-2586 - JMAP Upload - Fix unstable UploadService test
8cf5171 
- Method delete of Upload Repository should return Boolean value when applied
- optimize resetSpace method - single call replace to twice call (get & update)
- The auto cleanup upload when exceed do not ensure the concurrent -> we need sleep time after concurrent upload.
@vttranlina
Copy link
Contributor Author

Or I will cherry-pick a commit from the master

I did cherry-pick commits from master
869ee61
3e76f95
(and resolved conflict)

@Arsnael Arsnael merged commit b18ca30 into apache:postgresql Mar 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chibenwa chibenwa chibenwa left review comments

@quantranhong1999 quantranhong1999 quantranhong1999 left review comments

@Arsnael Arsnael Arsnael approved these changes

@hungphan227 hungphan227 hungphan227 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@vttranlina @Arsnael @quantranhong1999 @chibenwa @hungphan227