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-4071 - Task fix inconsistencies messagedeleted, mailboxRecents #2408

Merged
merged 2 commits into from
Sep 18, 2024
Merged

JAMES-4071 - Task fix inconsistencies messagedeleted, mailboxRecents #2408

merged 2 commits into from
Sep 18, 2024

Conversation

vttranlina
Copy link
Contributor

@vttranlina vttranlina commented Sep 12, 2024 •
edited
Loading

@vttranlina vttranlina mentioned this pull request Sep 12, 2024
Closed
chibenwa
chibenwa reviewed Sep 12, 2024
View reviewed changes
Copy link
Contributor

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

Overall good to me but it misses some sorts of tests

@vttranlina vttranlina changed the title WIP - Task fix inconsistencies messagedeleted @vttranlina JAMES-4071 - Task fix inconsistencies messagedeleted, mailboxRecents Sep 13, 2024
@vttranlina vttranlina changed the title @vttranlina JAMES-4071 - Task fix inconsistencies messagedeleted, mailboxRecents JAMES-4071 - Task fix inconsistencies messagedeleted, mailboxRecents Sep 13, 2024
@vttranlina vttranlina marked this pull request as ready for review September 13, 2024 08:37
@Arsnael
Copy link
Contributor

Arsnael commented Sep 13, 2024

08:54:41,418 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.0:check (check-style) on project apache-james-mailbox-cassandra: You have 1 Checkstyle violation. -> [Help 1]

@Arsnael
Copy link
Contributor

Arsnael commented Sep 13, 2024

09:42:22,092 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.0:check (check-style) on project rabbitmq-webadmin-integration-test: You have 2 Checkstyle violations. -> [Help 1]

Please check your checkstyle globally

chibenwa
chibenwa reviewed Sep 13, 2024
View reviewed changes
Copy link
Contributor

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

As a general comment I feel uneasy with the amount of code duplication.

I wonder if the verbosity of this PR could be reduced by "sharing" the task layer.

That is:

  • Keep 2 RequestToTask as today
  • Keep 2 'service' as today
  • Find a more generic name for the task like SolveMailboxFlagDenormalisationTask
  • Have a Type enum sayng if SolveMailboxFlagDenormalisationTask shall fix Recent or Deleted
  • Based on this enum field the task can choose the right service
  • And the webadmin layer just passes the right value downstream...

I counted, this alone could spare ~260 lines of code.

Perhaps even the service layer could be shared:

  • if the two DAO implement the same interface to add, deleteAll entries
  • then it would be straight foward to choose the DAO and the messageFilter based on the mode, sparing ~70 lines more

@vttranlina
Copy link
Contributor Author

Find a more generic name for the task like SolveMailboxFlagDenormalisationTask
Have a Type enum sayng if SolveMailboxFlagDenormalisationTask shall fix Recent or Deleted
Based on this enum field the task can choose the right service

@chibenwa

I tried with this way, but got an stuck with Task Serialize
TaskDTOModule<Task, TaskDTO>
The fact it still is 2 dedicate task, if we merge to one, i got an error guice injecttion
java.lang.IllegalArgumentException: Multiple entries with same key:

// The same issue with
Serialize TaskAdditionalInformation -> TaskAdditionalInformationDTO

@chibenwa
Copy link
Contributor

I do not understand with 1 task you are supposed to get only one dto module?

@vttranlina
Copy link
Contributor Author

I do not understand with 1 task you are supposed to get only one dto module?

yes,
I have an idea to bypass it, (just add one more json property...)
Wait for me to implement it.

chibenwa
chibenwa approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

Thanks!

@vttranlina
Copy link
Contributor Author

rebase master & squash fixup

@Arsnael Arsnael merged commit 31fb38f into apache:master Sep 18, 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 approved these changes

@Arsnael Arsnael Arsnael approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistencies: need a task to rebuild messagedeleted table
3 participants
@vttranlina @Arsnael @chibenwa