refact(core): adaptor for common 1.2 & fix a string of possible CI problem #286

imbajin · 2023-11-22T05:05:49Z

Purpose of the PR

subtask of feat(common): replace jersey dependencies with OkHttp (Breaking Change) incubator-hugegraph-commons#133 (upgrade common to v1.2)

Main Changes

Verifying these changes

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.

Does this PR potentially affect the following parts?

Documentation Status

Doc - TODO
Doc - Done
Doc - No Need

simon824 · 2023-11-22T06:45:02Z

Error:  /home/runner/work/incubator-hugegraph-computer/incubator-hugegraph-computer/computer-k8s-operator/src/main/java/org/apache/hugegraph/computer/k8s/operator/OperatorEntrypoint.java:[39,23] package org.apache.http does not exist

This exception in CI seems to be caused by common 1.2

imbajin · 2023-11-22T06:46:15Z

Error:  /home/runner/work/incubator-hugegraph-computer/incubator-hugegraph-computer/computer-k8s-operator/src/main/java/org/apache/hugegraph/computer/k8s/operator/OperatorEntrypoint.java:[39,23] package org.apache.http does not exist

This exception in CI seems to be caused by common 1.2

Yes, I'll adapt it soon (already marked in the commons PR)

Update: (such problem we could ask for coplit/LLM instead)

imbajin · 2023-11-23T10:30:29Z

this PR will update after toolchain upgrade to v1.2 (currently it has dependency conflicts)

imbajin · 2023-12-05T08:02:44Z

@coderzc we could replace all the download & start service script to docker run if the image exist like adf4c43

TODOs:

loader
hdfs (if we can't cache it)
etcd
....

This reverts commit 3e894d3.

coderzc · 2023-12-06T13:47:47Z

computer-test/src/main/java/org/apache/hugegraph/computer/core/worker/WorkerServiceTest.java

                masterService.init(config);
                masterService.execute();
            } catch (Throwable e) {
                LOG.error("Failed to start master", e);
                exceptions[2] = e;
            } finally {
-                masterService.close();
                countDownLatch.countDown();


We need to close the worker if master failed, otherwise the main thread won't be able to finish.

We need to close the worker if master failed, otherwise the main thread won't be able to finish.

Seems in this case, master close() in the end of the test? (and how could we close worker here)

Seems in this case, master close() in the end of the test?

I think so.

how could we close worker here

We can call workerService.close()

We can call workerService.close()

but in this case we can't call it because the worker service scope is not shared/enough (In other case we could do that)

coderzc · 2023-12-07T04:40:58Z

computer-core/src/main/java/org/apache/hugegraph/computer/core/master/MasterService.java

@@ -114,9 +113,9 @@ public void init(Config config) {
        LOG.info("{} register MasterService", this);
        this.bsp4Master.masterInitDone(this.masterInfo);

-        this.workers = this.bsp4Master.waitWorkersInitDone();
+        List<ContainerInfo> workers = this.bsp4Master.waitWorkersInitDone();


Why don't assign value to this.workers?

seems not used in other space.. (only defined in the global but not used besides here)

Oh, it's better to print the workers' information so troubleshooting.

computer-core/src/main/java/org/apache/hugegraph/computer/core/worker/WorkerService.java

computer-core/src/main/java/org/apache/hugegraph/computer/core/master/MasterService.java

…/worker/WorkerService.java Co-authored-by: Cong Zhao <zhaocong@apache.org>

javeme · 2023-12-09T15:27:21Z

checkstyle.xml

@@ -35,7 +35,7 @@
    <module name="TreeWalker">
        <!--检查行长度-->
        <module name="LineLength">
-            <property name="max" value="100"/>
+            <property name="max" value="105"/>


prefer don't break the rule

Update to 101 & add the comment

computer-core/src/main/java/org/apache/hugegraph/computer/core/master/MasterService.java

computer-core/src/main/java/org/apache/hugegraph/computer/core/worker/WorkerService.java

javeme · 2023-12-09T15:34:12Z

computer-core/src/main/java/org/apache/hugegraph/computer/core/worker/WorkerService.java

+            this.managers.initedAll(this.config);
+            LOG.info("{} WorkerService initialized", this);
+            this.inited = true;
+        } catch (Exception e) {


don't need to try-catch and just log here

don't need to try-catch and just log here

seems without try-catch the log won't print if exception throws before it

computer-core/src/main/java/org/apache/hugegraph/computer/core/worker/WorkerService.java

coderzc

LGTM

chore(ci): update common 1.2 & upgrade action version

06f1130

imbajin added this to the 1.2.0 milestone Nov 22, 2023

imbajin requested review from simon824 and coderzc November 22, 2023 05:05

coderzc previously approved these changes Nov 22, 2023

View reviewed changes

replace HTTP_CODE

7e8e021

imbajin dismissed coderzc’s stale review via 7e8e021 November 22, 2023 07:30

Update codeql-analysis.yml

b6d260c

imbajin marked this pull request as draft November 23, 2023 10:30

imbajin mentioned this pull request Dec 4, 2023

[Feature] Replace jersey in hugegraph-client/common to avoid SpringBoot/Jarkata conflicts apache/incubator-hugegraph-toolchain#505

Closed

2 tasks

imbajin added 2 commits December 5, 2023 15:28

Merge branch 'master' into pr/286

bf0dc10

Update pom.xml

568134f

imbajin marked this pull request as ready for review December 5, 2023 07:43

use docker image for server

adf4c43

imbajin added 12 commits December 5, 2023 18:27

update webmockserver version to adapt okhttp4.x

9d0e477

remove useless client

3e894d3

Revert "remove useless client"

b0f82c6

This reverts commit 3e894d3.

chore: replace loading data with docker & fix AssertThrows

9d0f779

update loader image name

327f446

Update load-data-into-hugegraph.sh

9a8f736

Update load-data-into-hugegraph.sh

32b4cb9

Update load-data-into-hugegraph.sh

eeaee79

change path

06343fd

Update load-data-into-hugegraph.sh

317e9ab

Update load-data-into-hugegraph.sh

3849038

Update load-data-into-hugegraph.sh

e85856f

imbajin added 8 commits December 5, 2023 20:35

update ci logic

91f300d

Update load-data-into-hugegraph.sh

55ceb52

Update load-data-into-hugegraph.sh

7a3d41a

Update load-data-into-hugegraph.sh

80811d4

Update WorkerServiceTest.java

126e115

Merge branch 'master' into pr/286

4d47f48

update data path

ce5dddc

refactor WorkerServer

0fad470

imbajin changed the title ~~chore(ci): update common 1.2 & upgrade action version~~ refact(core): adaptor for common 1.2 & fix a string of possible CI problem Dec 6, 2023

coderzc reviewed Dec 6, 2023

View reviewed changes

imbajin added 4 commits December 7, 2023 11:02

update the WokerService init & close logic

c2273cf

Update MasterService.java

8af9d5a

CI passed & revert the ignore

b223826

Update MessageRecvManagerTest.java

4f4b2c5

coderzc reviewed Dec 7, 2023

View reviewed changes

imbajin added 2 commits December 7, 2023 16:49

enable 3rd party check

80ac743

Update license-checker.yml

45c9add

coderzc reviewed Dec 7, 2023

View reviewed changes

computer-core/src/main/java/org/apache/hugegraph/computer/core/worker/WorkerService.java Outdated Show resolved Hide resolved

computer-core/src/main/java/org/apache/hugegraph/computer/core/worker/WorkerService.java Show resolved Hide resolved

coderzc reviewed Dec 7, 2023

View reviewed changes

computer-core/src/main/java/org/apache/hugegraph/computer/core/master/MasterService.java Show resolved Hide resolved

imbajin and others added 2 commits December 8, 2023 13:57

Update computer-core/src/main/java/org/apache/hugegraph/computer/core…

ce2849d

…/worker/WorkerService.java Co-authored-by: Cong Zhao <zhaocong@apache.org>

Update MasterService.java

7541287

imbajin mentioned this pull request Dec 8, 2023

chore: add check dependencies #293

Merged

11 tasks

javeme reviewed Dec 9, 2023

View reviewed changes

coderzc previously approved these changes Dec 10, 2023

View reviewed changes

tiny fix

7be91f2

imbajin dismissed coderzc’s stale review via 7be91f2 December 11, 2023 02:28

coderzc approved these changes Dec 11, 2023

View reviewed changes

simon824 approved these changes Dec 11, 2023

View reviewed changes

simon824 merged commit 89a8f9b into master Dec 11, 2023
7 checks passed

simon824 deleted the visit-stage branch December 11, 2023 07:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refact(core): adaptor for common 1.2 & fix a string of possible CI problem #286

refact(core): adaptor for common 1.2 & fix a string of possible CI problem #286

imbajin commented Nov 22, 2023 •

edited

Loading

simon824 commented Nov 22, 2023

imbajin commented Nov 22, 2023 •

edited

Loading

imbajin commented Nov 23, 2023 •

edited

Loading

imbajin commented Dec 5, 2023 •

edited

Loading

coderzc Dec 6, 2023 •

edited

Loading

imbajin Dec 7, 2023

coderzc Dec 7, 2023

imbajin Dec 7, 2023 •

edited

Loading

coderzc Dec 7, 2023

imbajin Dec 7, 2023 •

edited

Loading

coderzc Dec 7, 2023

javeme Dec 9, 2023

imbajin Dec 11, 2023

javeme Dec 9, 2023

imbajin Dec 11, 2023

coderzc left a comment

refact(core): adaptor for common 1.2 & fix a string of possible CI problem #286

refact(core): adaptor for common 1.2 & fix a string of possible CI problem #286

Conversation

imbajin commented Nov 22, 2023 • edited Loading

Purpose of the PR

Main Changes

Verifying these changes

Does this PR potentially affect the following parts?

Documentation Status

simon824 commented Nov 22, 2023

imbajin commented Nov 22, 2023 • edited Loading

imbajin commented Nov 23, 2023 • edited Loading

imbajin commented Dec 5, 2023 • edited Loading

coderzc Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imbajin Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imbajin Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderzc left a comment

Choose a reason for hiding this comment

imbajin commented Nov 22, 2023 •

edited

Loading

imbajin commented Nov 22, 2023 •

edited

Loading

imbajin commented Nov 23, 2023 •

edited

Loading

imbajin commented Dec 5, 2023 •

edited

Loading

coderzc Dec 6, 2023 •

edited

Loading

imbajin Dec 7, 2023 •

edited

Loading

imbajin Dec 7, 2023 •

edited

Loading