You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "qwtsc (via GitHub)" <gi...@apache.org> on 2023/11/23 00:22:46 UTC
[PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
qwtsc opened a new pull request, #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290
<!--
Thank you very much for contributing to Apache HugeGraph, we are happy that you want to help us improve it!
Here are some tips for you:
1. If this is your first time, please read the [contributing guidelines](https://github.com/apache/hugegraph/blob/master/CONTRIBUTING.md)
2. If a PR will fix/close a issue, type the message "close xxx" (xxx is the link of related issue) in the content, github will auto link it (Required)
3. Name the PR title in "Google Commit Format", start with "feat | fix | perf | refactor | doc | chore",
such like: "feat(core): support the PageRank algorithm" or "fix: wrong break in the compute loop" (module is optional)
skip it if you are unsure about which is the best component.
4. One PR address one issue, better not to mix up multiple issues.
5. Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]` (or click it directly after published)
-->
## Purpose of the PR
- close #xxx <!-- or use "fix #xxx", "xxx" is the ID-link of related issue, e.g: close #257 -->
- for debug testcase
<!--
Please explain more context in this section, clarify why the changes are needed.
For example:
- If you propose a new API, clarify the use case for a new API.
- If you fix a bug, you can clarify why it is a bug, and should associated with an issue.
-->
## Main Changes
<!-- Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue. These change logs are helpful for better ant faster reviews.)
For example:
- If you introduce a new feature, please show detailed design here or add the link of design documentation.
- If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
- If there is a discussion in the mailing list, please add the link. -->
## Verifying these changes
<!-- Please pick the proper options below -->
- [x ] Trivial rework / code cleanup without any test coverage. (No Need)
- [ ] Already covered by existing tests, such as *(please modify tests here)*.
- [ ] Need tests and can be verified as follows.
<!-- Please provide more details about verification
For example:
- If you test manually, please provide related screenshot.
-->
## Does this PR potentially affect the following parts?
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [x ] Nope
- [ ] Dependencies (add/update license info) <!-- Don't forget to add/update the info in "LICENSE" & "NOTICE" files (both in root & dist module) -->
- [ ] Modify configurations
- [ ] The public API
- [ ] Other affects (typed here)
## Documentation Status
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `Doc - TODO` <!-- Your PR changes impact docs and you will update later -->
- [ ] `Doc - Done` <!-- Related docs have been already added or updated -->
- [x ] `Doc - No Need` <!-- Your PR changes don't impact/need docs -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "qwtsc (via GitHub)" <gi...@apache.org>.
qwtsc commented on PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#issuecomment-1827323729
@javeme @imbajin I finally find the actual reason.
* the default delete policy in k8s has changed to `background` since v1.20.
* Plus the way you added finalizer is `replaceCR`, so you will sometimes miss the delete event generated by your client.
```java
private boolean finalizer(HugeGraphComputerJob computerJob) {
if (computerJob.addFinalizer(FINALIZER_NAME)) {
---> // this is partial reason, can u use patch CR instead?
this.replaceCR(computerJob);
return true;
}
ComputerJobStatus status = computerJob.getStatus();
if (computerJob.isMarkedForDeletion()) {
if (!JobStatus.finished(status.getJobStatus())) {
status.setJobStatus(JobStatus.CANCELLED.name());
this.updateStatus(computerJob);
} else {
if (computerJob.removeFinalizer(FINALIZER_NAME)) {
this.replaceCR(computerJob);
}
}
return true;
} else {
if (JobStatus.finished(status.getJobStatus())) {
if (this.autoDestroyPod) {
this.deleteCR(computerJob);
}
return true;
}
}
return false;
}
```
* k8s will deleted its owned resource in the background whether the CR is deleted or not, so the calculated job status will always remain INITIALIZING, then the operator won't deploy again.
```java
ComputerJobComponent observed = this.observeComponent(computerJob);
----> //calculated status remains INITIALIZING forever, so won't deploy it again.
if (!this.updateStatus(observed) && request.retryTimes() == 0) {
LOG.debug("Wait status to be stable before taking further actions");
return OperatorResult.NO_REQUEUE;
}
if (Objects.equals(computerJob.getStatus().getJobStatus(),
JobStatus.RUNNING.name())) {
String crName = computerJob.getMetadata().getName();
LOG.info("ComputerJob {} already running, no action", crName);
return OperatorResult.NO_REQUEUE;
}
ComputerJobDeployer deployer = new ComputerJobDeployer(this.kubeClient,
this.config);
deployer.deploy(observed);
return OperatorResult.NO_REQUEUE;
```
* Then as human, we observe the unit-test case failed again and again.
### solution
My solution is to adjust the delete policy back to foreground. Everyone is happy now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#discussion_r1403021831
##########
computer-driver/src/main/java/org/apache/hugegraph/computer/driver/JobStatus.java:
##########
@@ -18,6 +18,7 @@
package org.apache.hugegraph.computer.driver;
import java.util.Objects;
+import java.util.StringJoiner;
Review Comment:
unused?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] feat: use 'foreground' delete policy to pass the cancel job test [incubator-hugegraph-computer]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin merged PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] feat: use `foreground` delete policy rather than default `background` option to pass the test case of `cancel job` [incubator-hugegraph-computer]
Posted by "qwtsc (via GitHub)" <gi...@apache.org>.
qwtsc commented on PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#issuecomment-1838187574
> @qwtsc Please correct the title of PR.
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "qwtsc (via GitHub)" <gi...@apache.org>.
qwtsc commented on code in PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#discussion_r1405699048
##########
computer-driver/src/main/java/org/apache/hugegraph/computer/driver/DefaultJobState.java:
##########
@@ -87,4 +88,14 @@ public int hashCode() {
return Objects.hash(this.superstep, this.maxSuperstep,
this.lastSuperstepStat, this.jobStatus);
}
+
+ @Override
+ public String toString() {
+ return new StringJoiner(", ", DefaultJobState.class.getSimpleName() + "[", "]")
+ .add("superstep=" + superstep)
+ .add("maxSuperstep=" + maxSuperstep)
+ .add("lastSuperstepStat=" + lastSuperstepStat)
+ .add("jobStatus=" + jobStatus)
+ .toString();
Review Comment:
This is generated by IDEA. If you're stick to using String.format(), I will change it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#discussion_r1404583901
##########
computer-driver/src/main/java/org/apache/hugegraph/computer/driver/DefaultJobState.java:
##########
@@ -87,4 +88,14 @@ public int hashCode() {
return Objects.hash(this.superstep, this.maxSuperstep,
this.lastSuperstepStat, this.jobStatus);
}
+
+ @Override
+ public String toString() {
+ return new StringJoiner(", ", DefaultJobState.class.getSimpleName() + "[", "]")
+ .add("superstep=" + superstep)
+ .add("maxSuperstep=" + maxSuperstep)
+ .add("lastSuperstepStat=" + lastSuperstepStat)
+ .add("jobStatus=" + jobStatus)
+ .toString();
Review Comment:
maybe using String.format() is more clear?
##########
computer-driver/src/main/java/org/apache/hugegraph/computer/driver/JobStatus.java:
##########
@@ -36,4 +36,5 @@ public static boolean finished(String status) {
Objects.equals(status, FAILED.name()) ||
Objects.equals(status, SUCCEEDED.name());
}
+
Review Comment:
revert it?
##########
computer-test/src/main/java/org/apache/hugegraph/computer/k8s/MiniKubeTest.java:
##########
@@ -243,7 +243,7 @@ public void testJobCancelled() {
jobObserver);
DefaultJobState jobState = new DefaultJobState();
- jobState.jobStatus(JobStatus.INITIALIZING);
+ jobState.jobStatus(JobStatus.RUNNING);
Review Comment:
also sleep UnitTestBase.sleep(1000L) to ensure RUNNING status is stable?
can we push this code to a separated PR or append to this PR https://github.com/apache/incubator-hugegraph-computer/pull/287 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "qwtsc (via GitHub)" <gi...@apache.org>.
qwtsc commented on PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#issuecomment-1827232720
I still can't find the root cause. Let's keep this PR open for another two or three days. If it's still mysterious to me, we can append it to PR #287.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "qwtsc (via GitHub)" <gi...@apache.org>.
qwtsc commented on code in PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#discussion_r1405695595
##########
computer-test/src/main/java/org/apache/hugegraph/computer/k8s/MiniKubeTest.java:
##########
@@ -243,7 +243,7 @@ public void testJobCancelled() {
jobObserver);
DefaultJobState jobState = new DefaultJobState();
- jobState.jobStatus(JobStatus.INITIALIZING);
+ jobState.jobStatus(JobStatus.RUNNING);
Review Comment:
I don't think so. You don't need sleep another one second.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "qwtsc (via GitHub)" <gi...@apache.org>.
qwtsc commented on PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#issuecomment-1825163496
> maybe we need to implement `DefaultJobState.toString()`?
Done, and I have fond the issue that blocked the minikube test. please review the latest changed files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "coderzc (via GitHub)" <gi...@apache.org>.
coderzc commented on PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#issuecomment-1824051891
<img width="739" alt="image" src="https://github.com/apache/incubator-hugegraph-computer/assets/26179648/4cf13900-99bb-4881-8076-f569b7800b7f">
It seems Enum already overrides write `toString`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "qwtsc (via GitHub)" <gi...@apache.org>.
qwtsc commented on PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#issuecomment-1829490383
> BTW, the u could import the [hugegraph-style.xml](https://github.com/apache/incubator-hugegraph/wiki/The-configuration-for-HugeGraph-in-IDEA-(BETA)#%E7%AC%AC%E4%B8%80%E6%AD%A5-%E9%80%9A%E7%94%A8%E9%85%8D%E7%BD%AE) to avoid most of the align/import problems
Gotcha.
I cannot repeat your failure, but it failed with another reason. I am concerned with the quality of test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#issuecomment-1829029986
<img width="1079" alt="image" src="https://github.com/apache/incubator-hugegraph-computer/assets/17706099/529a0ae6-844a-4a35-a37b-49428242e6b2">
retry
<img width="1089" alt="image" src="https://github.com/apache/incubator-hugegraph-computer/assets/17706099/0ca318ce-a676-40bc-a38e-766da8bfbab3">
https://github.com/apache/incubator-hugegraph-computer/blob/0c447be2c61a521fbe6cd5fc5f1273218ba268d3/computer-test/src/main/java/org/apache/hugegraph/computer/core/network/netty/NettyTransportClientTest.java#L316
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#issuecomment-1825586205
## [Codecov](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `9 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`ff85e34`)](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/commit/ff85e34efaecb2a14ae72c0048a427a0aeb61bf3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 85.03% compared to head [(`08a387a`)](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 85.07%.
> Report is 3 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...egraph/computer/algorithm/sampling/RandomWalk.java](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tcHV0ZXItYWxnb3JpdGhtL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWdlZ3JhcGgvY29tcHV0ZXIvYWxnb3JpdGhtL3NhbXBsaW5nL1JhbmRvbVdhbGsuamF2YQ==) | 92.06% | [2 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...thm/centrality/betweenness/BetweennessMessage.java](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tcHV0ZXItYWxnb3JpdGhtL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWdlZ3JhcGgvY29tcHV0ZXIvYWxnb3JpdGhtL2NlbnRyYWxpdHkvYmV0d2Vlbm5lc3MvQmV0d2Vlbm5lc3NNZXNzYWdlLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../algorithm/community/cc/ClusteringCoefficient.java](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tcHV0ZXItYWxnb3JpdGhtL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWdlZ3JhcGgvY29tcHV0ZXIvYWxnb3JpdGhtL2NvbW11bml0eS9jYy9DbHVzdGVyaW5nQ29lZmZpY2llbnQuamF2YQ==) | 0.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...computer/algorithm/sampling/RandomWalkMessage.java](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tcHV0ZXItYWxnb3JpdGhtL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWdlZ3JhcGgvY29tcHV0ZXIvYWxnb3JpdGhtL3NhbXBsaW5nL1JhbmRvbVdhbGtNZXNzYWdlLmphdmE=) | 94.11% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #290 +/- ##
============================================
+ Coverage 85.03% 85.07% +0.04%
- Complexity 3246 3278 +32
============================================
Files 345 349 +4
Lines 12298 12412 +114
Branches 1102 1113 +11
============================================
+ Hits 10458 10560 +102
- Misses 1315 1320 +5
- Partials 525 532 +7
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-hugegraph-computer/pull/290?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#discussion_r1409305830
##########
computer-driver/src/main/java/org/apache/hugegraph/computer/driver/DefaultJobState.java:
##########
@@ -87,4 +87,11 @@ public int hashCode() {
return Objects.hash(this.superstep, this.maxSuperstep,
this.lastSuperstepStat, this.jobStatus);
}
+
+ @Override
+ public String toString() {
+ return String.format("%s[super=%s, maxSuperStep=%s, lastSuperstepStat=%s, jobStatus=%s]",
Review Comment:
super=>superstep
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "coderzc (via GitHub)" <gi...@apache.org>.
coderzc commented on PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#issuecomment-1836068255
@qwtsc Please correct the title of PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#issuecomment-1824439135
maybe we need to implement `DefaultJobState.toString()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#issuecomment-1825999272
> thanks and could give more context about the issue/bug?
it may be this issue: https://github.com/apache/incubator-hugegraph-computer/pull/287#discussion_r1403393492
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#discussion_r1405843153
##########
computer-k8s/src/main/java/org/apache/hugegraph/computer/k8s/driver/KubernetesDriver.java:
##########
@@ -282,7 +283,9 @@ private void checkComputerConf(Map<String, String> computerConf,
@Override
public boolean cancelJob(String jobId, Map<String, String> params) {
- return this.operation.withName(KubeUtil.crName(jobId)).delete();
+ return this.operation.withName(KubeUtil.crName(jobId))
+ .withPropagationPolicy(DeletionPropagation.FOREGROUND)
Review Comment:
prefer to align with `.withName`
##########
computer-driver/src/main/java/org/apache/hugegraph/computer/driver/DefaultJobState.java:
##########
@@ -87,4 +88,14 @@ public int hashCode() {
return Objects.hash(this.superstep, this.maxSuperstep,
this.lastSuperstepStat, this.jobStatus);
}
+
+ @Override
+ public String toString() {
+ return new StringJoiner(", ", DefaultJobState.class.getSimpleName() + "[", "]")
+ .add("superstep=" + superstep)
+ .add("maxSuperstep=" + maxSuperstep)
+ .add("lastSuperstepStat=" + lastSuperstepStat)
+ .add("jobStatus=" + jobStatus)
+ .toString();
Review Comment:
yes prefer String.format since it does not affect performance. Thanks~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "qwtsc (via GitHub)" <gi...@apache.org>.
qwtsc commented on code in PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#discussion_r1405699048
##########
computer-driver/src/main/java/org/apache/hugegraph/computer/driver/DefaultJobState.java:
##########
@@ -87,4 +88,14 @@ public int hashCode() {
return Objects.hash(this.superstep, this.maxSuperstep,
this.lastSuperstepStat, this.jobStatus);
}
+
+ @Override
+ public String toString() {
+ return new StringJoiner(", ", DefaultJobState.class.getSimpleName() + "[", "]")
+ .add("superstep=" + superstep)
+ .add("maxSuperstep=" + maxSuperstep)
+ .add("lastSuperstepStat=" + lastSuperstepStat)
+ .add("jobStatus=" + jobStatus)
+ .toString();
Review Comment:
This is generated by IDEA. If you stick to using String.format(), I will change it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore: add toString in jobstatus [incubator-hugegraph-computer]
Posted by "qwtsc (via GitHub)" <gi...@apache.org>.
qwtsc commented on code in PR #290:
URL: https://github.com/apache/incubator-hugegraph-computer/pull/290#discussion_r1405699885
##########
computer-driver/src/main/java/org/apache/hugegraph/computer/driver/JobStatus.java:
##########
@@ -36,4 +36,5 @@ public static boolean finished(String status) {
Objects.equals(status, FAILED.name()) ||
Objects.equals(status, SUCCEEDED.name());
}
+
Review Comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org