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

Fix compile warnings in mantis server worker #423

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Fix compile warnings in mantis server worker #423

wants to merge 13 commits into from

Conversation

SalmaAfifi
Copy link

@SalmaAfifi SalmaAfifi commented Apr 18, 2023 •
edited
Loading

Context

Resolves #394

Checklist

  • ./gradlew build compiles code correctly
  • Added new tests where applicable
  • ./gradlew test passes all tests
  • Extended README or added javadocs where applicable

@SalmaAfifi SalmaAfifi had a problem deploying to Integrate Pull Request April 18, 2023 13:01 — with GitHub Actions Failure
@github-actions
Copy link

Test Results

126 files  ±0  126 suites  ±0   6m 18s ⏱️ +4s
535 tests ±0  526 ✔️ ±0  8 💤 ±0  1 ❌ ±0 
536 runs  ±0  527 ✔️ ±0  8 💤 ±0  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit d2008bd. ± Comparison against base commit 823474f.

liuml07
liuml07 reviewed Apr 22, 2023
View reviewed changes
Copy link
Contributor

@liuml07 liuml07 left a comment

Choose a reason for hiding this comment

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

Good work!

I provided some early comments. Will take a look next week. Thanks

mantis-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/RunningWorker.java Outdated
}
this.onTerminateCallback = blockUntilTerminate::countDown;
this.onCompleteCallback = () -> {
logger.info("JobId: " + jobId + " stage: " + stageNum + ", completed");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we replace string concatenation with parameterized / placeholder in logging? It applies to other logging lines in this module.

logger.info("JobId: {} stage: {} completed", jobId, stageNum);

See this doc for advantages: https://www.tutorialspoint.com/slf4j/slf4j_parameterized_logging.htm

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Fixed in e9ad7a8

...ver-worker/src/main/java/io/mantisrx/server/worker/VirtualMachineWorkerServiceLocalImpl.java
}
Schedulers.newThread().createWorker().schedule(() -> {
try {
WrappedExecuteStageRequest request;
Copy link
Contributor

Choose a reason for hiding this comment

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

merge the two lines

Copy link
Author

Choose a reason for hiding this comment

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

If you mean to merge them like that
Schedulers.newThread().createWorker().schedule(() -> { try {
It looks more well-formated to me with the line break.

...ver-worker/src/main/java/io/mantisrx/server/worker/VirtualMachineWorkerServiceLocalImpl.java Outdated
logger.info("onNext'ing WrappedExecuteStageRequest: {}", request);
executeStageRequestObserver.onNext(request);
} catch (MalformedURLException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use logger.error and attach request information

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 4df01c3
But I think the request would be null in this case. Because the exception would be thrown during the initialization of the request.

...ver-worker/src/main/java/io/mantisrx/server/worker/VirtualMachineWorkerServiceLocalImpl.java Outdated
}
vmTaskStatusObservable.subscribe(vmTaskStatus -> {
TYPE type = vmTaskStatus.getType();
if (type == TYPE.COMPLETED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's replace this with switch-case

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 4df01c3

...-server/mantis-server-worker/src/main/java/io/mantisrx/server/worker/WorkerIndexHistory.java Outdated
@@ -22,8 +22,8 @@

public class WorkerIndexHistory {

final Set<Integer> runningWorkerIndex = new HashSet<Integer>();
final Set<Integer> terminalWorkerIndex = new HashSet<Integer>();
final Set<Integer> runningWorkerIndex = new HashSet<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be private?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 0b3695b

...ver-worker/src/main/java/io/mantisrx/server/worker/jobmaster/clutch/ClutchConfiguration.java
@@ -119,38 +120,38 @@ public boolean equals(Object o) {
if (Double.compare(this.getRps(), other.getRps()) != 0) return false;
final Object this$minSamples = this.getMinSamples();
final Object other$minSamples = other.getMinSamples();
if (this$minSamples == null ? other$minSamples != null : !this$minSamples.equals(other$minSamples))
if (!Objects.equals(this$minSamples, other$minSamples))
Copy link
Contributor

Choose a reason for hiding this comment

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

@sundargates this code is too verbose. I'm wondering if it's a good idea to pull in commons-lang3 dependency and use EqualsBuilder() for this.

...erver-worker/src/main/java/io/mantisrx/server/worker/mesos/MesosExecutorCallbackHandler.java
}
};
Thread t = new Thread(() -> {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

line break seems odd here.

...orker/src/main/java/io/mantisrx/server/worker/mesos/VirualMachineWorkerServiceMesosImpl.java Outdated
}
vmTaskStatusObservable.subscribe(vmTaskStatus -> {
TYPE type = vmTaskStatus.getType();
if (type == TYPE.COMPLETED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use switch-case?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 4526f92

mantis-server/mantis-server-worker/src/test/java/io/mantisrx/server/worker/HeartbeatTest.java
payloads = heartbeat.getCurrentHeartbeatStatus().getPayloads();
Assert.assertEquals(1, payloads.size());
assertEquals(1, payloads.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets log the payloads before asserting the size, so when the test fails, it's clearer why.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Fixed in 07ad134

@SalmaAfifi SalmaAfifi had a problem deploying to Integrate Pull Request April 24, 2023 07:04 — with GitHub Actions Failure
@SalmaAfifi SalmaAfifi had a problem deploying to Integrate Pull Request April 24, 2023 08:16 — with GitHub Actions Failure
@SalmaAfifi SalmaAfifi had a problem deploying to Integrate Pull Request April 24, 2023 11:13 — with GitHub Actions Failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liuml07 liuml07 liuml07 left review comments

@nickmahilani nickmahilani Awaiting requested review from nickmahilani

@jeffchao jeffchao Awaiting requested review from jeffchao

@piygoyal piygoyal Awaiting requested review from piygoyal

@calvin681 calvin681 Awaiting requested review from calvin681 calvin681 is a code owner

@sundargates sundargates Awaiting requested review from sundargates sundargates is a code owner

@Andyz26 Andyz26 Awaiting requested review from Andyz26 Andyz26 is a code owner

@hmitnflx hmitnflx Awaiting requested review from hmitnflx hmitnflx is a code owner

@markcho markcho Awaiting requested review from markcho

@fdc-ntflx fdc-ntflx Awaiting requested review from fdc-ntflx fdc-ntflx is a code owner

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

Fix compile warnings in mantis-server-worker module
2 participants
@SalmaAfifi @liuml07