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

[Improvement-6540][server] Optimize the code related to the processInstanceExecMaps object #6546

Closed
wants to merge 10 commits into from
Closed

[Improvement-6540][server] Optimize the code related to the processInstanceExecMaps object #6546

wants to merge 10 commits into from

Conversation

eye-gu
Copy link
Contributor

@eye-gu eye-gu commented Oct 15, 2021

Purpose of the pull request

Optimize the code related to the processInstanceExecMaps object

Brief change log

  • add ProcessInstanceExecCacheManager
  • change processInstanceExecMaps object related class

Verify this pull request

  • *Added ProcessInstanceExecCacheManagerImplTest

ruanwenjun
ruanwenjun requested changes Oct 15, 2021
View reviewed changes
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

Just some minor changed.

...rg/apache/dolphinscheduler/server/master/cache/impl/ProcessInstanceExecCacheManagerImpl.java Outdated Show resolved Hide resolved
...rg/apache/dolphinscheduler/server/master/cache/impl/ProcessInstanceExecCacheManagerImpl.java Outdated Show resolved Hide resolved
...r/src/main/java/org/apache/dolphinscheduler/server/master/registry/MasterRegistryClient.java Outdated Show resolved Hide resolved
...rver/src/main/java/org/apache/dolphinscheduler/server/master/runner/EventExecuteService.java Outdated Show resolved Hide resolved
...rver/src/main/java/org/apache/dolphinscheduler/server/master/runner/EventExecuteService.java Outdated Show resolved Hide resolved
...in/java/org/apache/dolphinscheduler/server/master/cache/ProcessInstanceExecCacheManager.java Show resolved Hide resolved
@ruanwenjun
Copy link
Member

Please fix the code style, you can import the checkstyle.xml file to your IDE.

@eye-gu
Copy link
Contributor Author

eye-gu commented Oct 16, 2021

sry about submitted multiple times, because I don't know checkstyle.
i do not change the failed unit test, how can i fixed it?

@ruanwenjun
Copy link
Member

@ououtt I am not sure if the UT failed is caused by your changes, you can rebase your code and submit again.

@eye-gu
Copy link
Contributor Author

eye-gu commented Oct 18, 2021

I found that I modified the constructor of the MasterRegistryClient class, it initialize the RegistryClient instance. Should I use an init method to initialize the MasterRegistryClient to make UT pass?

@ruanwenjun
Copy link
Member

@ououtt I think you can restore all the init method, your change shouldn't change the lifecycle of the components.

@eye-gu eye-gu closed this Oct 18, 2021
@eye-gu eye-gu deleted the Fix-6540 branch October 18, 2021 05:59
@eye-gu
Copy link
Contributor Author

eye-gu commented Oct 18, 2021

Thank you for your instruction. I deleted the original branch and resubmitted a new pr: 6556 .Maybe I can do better next time. @ruanwenjun

@snyk-bot snyk-bot mentioned this pull request Apr 7, 2022
Open
@William-GuoWei William-GuoWei mentioned this pull request May 13, 2022
Open
@William-GuoWei William-GuoWei mentioned this pull request Jun 21, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruanwenjun ruanwenjun ruanwenjun requested changes

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

Successfully merging this pull request may close these issues.
2 participants
@eye-gu @ruanwenjun