async watch the k8s job #148
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
javeme
reviewed
Nov 17, 2021
| @@ -426,6 +419,11 @@ public String log(String jobId, int containerId, long offset, long length, | |||
|
|
|||
| @Override | |||
| public void close() { | |||
| for (String jobId : this.waits.keySet()) { | |||
There was a problem hiding this comment.
can iterate this.waits.values()
|
|
||
| this.driver.cancelJob(jobId, params); | ||
| this.driver.waitJob(jobId, params, jobObserver); | ||
| CompletableFuture<Void> watchJob = this.driver.watchJob(jobId, params, |
There was a problem hiding this comment.
what's the behavior after cancel and re-watch?
| @@ -287,13 +286,14 @@ public void cancelJob(String jobId, Map<String, String> params) { | |||
| } | |||
|
|
|||
| @Override | |||
| public void waitJob(String jobId, Map<String, String> params, | |||
| JobObserver observer) { | |||
| public CompletableFuture<Void> watchJob(String jobId, | |||
There was a problem hiding this comment.
prefer keep waitJob() name or waitJobAsync
| @@ -139,7 +123,7 @@ public void testJobSucceed() { | |||
| Mockito.verify(jobObserver, Mockito.timeout(15000L).atLeast(1)) | |||
| .onJobStateChanged(Mockito.eq(jobState2)); | |||
|
|
|||
| future.getNow(null); | |||
| future.cancel(true); | |||
There was a problem hiding this comment.
why not wait for success with future.get()?
javeme
previously approved these changes
Nov 18, 2021
Codecov Report
@@ Coverage Diff @@
## master #148 +/- ##
============================================
- Coverage 87.88% 87.35% -0.54%
+ Complexity 2916 2906 -10
============================================
Files 306 308 +2
Lines 10982 10975 -7
Branches 930 930
============================================
- Hits 9652 9587 -65
- Misses 880 944 +64
+ Partials 450 444 -6
Continue to review full report at Codecov.
|
|
we need also update the version in |
imbajin
reviewed
Nov 19, 2021
| @@ -56,9 +57,11 @@ | |||
| * job is waiting by another thread. | |||
| * @param params reserved for other parameters in addition to jobId used | |||
| * to wait job. | |||
| * @return future for watch the job | |||
There was a problem hiding this comment.
in line 52
void cancelJob() --> boolean cancelJob is better? (so as the impl)
coderzc
force-pushed
the
async_k8s_wait
branch
from
9fba71f
to
5578d88
Compare
imbajin
reviewed
Nov 23, 2021
computer-k8s-operator/pom.xml
Outdated
| @@ -15,7 +15,7 @@ | |||
| <dependency> | |||
| <groupId>com.baidu.hugegraph</groupId> | |||
| <artifactId>computer-k8s</artifactId> | |||
| <version>${project.version}</version> | |||
| <version>0.1.1</version> | |||
There was a problem hiding this comment.
keep original param & modify the parent version
coderzc
force-pushed
the
async_k8s_wait
branch
from
5578d88
to
ae26f55
Compare
* update version to 0.1.1
coderzc
force-pushed
the
async_k8s_wait
branch
from
ae26f55
to
6c4a615
Compare
javeme
approved these changes
Nov 23, 2021
imbajin
approved these changes
Nov 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.