celeborn icon indicating copy to clipboard operation
celeborn copied to clipboard

[CELEBORN-1599] Container Info REST API

Open akpatnam25 opened this issue 1 year ago • 8 comments

What changes were proposed in this pull request?

Adding REST api and cli for container info. User can configure this api to be based on whichever cluster manager they are using.

Why are the changes needed?

see above

Does this PR introduce any user-facing change?

No

How was this patch tested?

added UTs

akpatnam25 avatar Sep 24 '24 05:09 akpatnam25

ping @FMX @SteNicholas @waitinfuture @turboFei PTAL thanks. I am not sure regarding the dependency check issues failing above, I ran ./dev/dependencies.sh --replace , but it is still failing. Is there anything else required to do?

akpatnam25 avatar Sep 24 '24 17:09 akpatnam25

ping @mridulm

akpatnam25 avatar Sep 24 '24 17:09 akpatnam25

ping @FMX @SteNicholas @waitinfuture @turboFei PTAL thanks. I am not sure regarding the dependency check issues failing above, I ran ./dev/dependencies.sh --replace , but it is still failing. Is there anything else required to do?

You need to fix dependencies for all modules. For example ./dev/dependencies.sh --replace --module spark-3.3

FMX avatar Sep 25 '24 08:09 FMX

I think this PR should change its structure to avoid unnecessary dependency diffusion. The openapi-client won't be needed in client modules.

updated to keep the logic in the service module @FMX , so no more dependency changes required in common.

akpatnam25 avatar Sep 26 '24 07:09 akpatnam25

ping @FMX @SteNicholas @mridulm PTAL. thanks

akpatnam25 avatar Oct 01 '24 06:10 akpatnam25

China is celebrating its seven-day National Day holidays

turboFei avatar Oct 02 '24 01:10 turboFei

China is celebrating its seven-day National Day holidays

sure, thanks for letting me know @turboFei

akpatnam25 avatar Oct 02 '24 18:10 akpatnam25

Codecov Report

Attention: Patch coverage is 4.69799% with 142 lines in your changes missing coverage. Please review.

Project coverage is 33.05%. Comparing base (9b83587) to head (1a9a2f5). Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
...g/apache/celeborn/rest/v1/model/ContainerInfo.java 0.00% 76 Missing :warning:
...org/apache/celeborn/rest/v1/master/DefaultApi.java 0.00% 18 Missing :warning:
...org/apache/celeborn/rest/v1/worker/DefaultApi.java 0.00% 18 Missing :warning:
...ommon/container/DefaultContainerInfoProvider.scala 0.00% 10 Missing :warning:
.../scala/org/apache/celeborn/common/util/Utils.scala 0.00% 9 Missing :warning:
...erver/common/container/ContainerInfoProvider.scala 0.00% 8 Missing :warning:
...cala/org/apache/celeborn/common/CelebornConf.scala 87.50% 1 Missing :warning:
...he/celeborn/common/identity/IdentityProvider.scala 0.00% 1 Missing :warning:
.../server/common/http/api/v1/ApiV1BaseResource.scala 0.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2758      +/-   ##
==========================================
- Coverage   33.13%   33.05%   -0.07%     
==========================================
  Files         314      317       +3     
  Lines       18435    18730     +295     
  Branches     1691     1719      +28     
==========================================
+ Hits         6106     6189      +83     
- Misses      11988    12200     +212     
  Partials      341      341              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 02 '24 18:10 codecov[bot]

@FMX sorry for the delay in responding, I was out on vacation. We need the reflection, because the DefaultContainerInfoProvider is the default impl. Users would supply their own implementation based on their environment and how they would retrieve the container info. For example, in a k8s env, retrieving the container info would involve hitting the k8s api server or getting it from env variables. This is different for each env, and is up to the user to provide.

akpatnam25 avatar Oct 15 '24 06:10 akpatnam25

Thanks. Merged into main(v0.6.0).

FMX avatar Oct 17 '24 09:10 FMX