[CELEBORN-1599] Container Info REST API

[akpatnam25]

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]

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]

ping @mridulm

[FMX]

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]

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

[akpatnam25]

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]

ping @FMX @SteNicholas @mridulm PTAL. thanks

[turboFei]

China is celebrating its seven-day National Day holidays

[akpatnam25]

China is celebrating its seven-day National Day holidays

sure, thanks for letting me know @turboFei

[codecov]

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 ⚠️
...org/apache/celeborn/rest/v1/master/DefaultApi.java	0.00%	18 Missing ⚠️
...org/apache/celeborn/rest/v1/worker/DefaultApi.java	0.00%	18 Missing ⚠️
...ommon/container/DefaultContainerInfoProvider.scala	0.00%	10 Missing ⚠️
.../scala/org/apache/celeborn/common/util/Utils.scala	0.00%	9 Missing ⚠️
...erver/common/container/ContainerInfoProvider.scala	0.00%	8 Missing ⚠️
...cala/org/apache/celeborn/common/CelebornConf.scala	87.50%	1 Missing ⚠️
...he/celeborn/common/identity/IdentityProvider.scala	0.00%	1 Missing ⚠️
.../server/common/http/api/v1/ApiV1BaseResource.scala	0.00%	1 Missing ⚠️

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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[FMX]

IMO, I don't see any relative reasons to use reflection for ContainerInfoProvider . Creating a new instance could be enough.

[akpatnam25]

@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.

[FMX]

LGTM.

[FMX]

Thanks. Merged into main(v0.6.0).