Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#951] build(operator): Add the HADOOP_VERSION for Docker image. #1027

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

qijiale76
Copy link
Contributor

@qijiale76 qijiale76 commented Jul 20, 2023

What changes were proposed in this pull request?

Add the HADOOP_VERSION for Docker image.

Why are the changes needed?

Fix: #951

Does this PR introduce any user-facing change?

No.

How was this patch tested?

By hand.

@jerqi jerqi changed the title [#951] fix: rss-server Docker image building bug. [#951] fix(operator): rss-server Docker image building bug. Jul 20, 2023
@jerqi jerqi changed the title [#951] fix(operator): rss-server Docker image building bug. [#951] fix(operator): Fix the issues of rss-server Docker image building. Jul 20, 2023
@jerqi
Copy link
Contributor

jerqi commented Jul 20, 2023

@qijiale76 Could you correct the description of pr?

@jerqi jerqi changed the title [#951] fix(operator): Fix the issues of rss-server Docker image building. [#951] build(operator): Add the HADOOP_VERSION for Docker image. Jul 20, 2023
@jerqi
Copy link
Contributor

jerqi commented Jul 20, 2023

@connorlwilkes Could you help review this pr if you have time?

@zuston
Copy link
Member

zuston commented Jul 21, 2023

I'm not sure is it necessary to bind hadoop version with image. If user don't use HDFS, this is weird

@jerqi
Copy link
Contributor

jerqi commented Jul 21, 2023

I'm not sure is it necessary to bind hadoop version with image. If user don't use HDFS, this is weird

Shuffle Server must use the HDFS jars. Because some configuration use HDFS jars.

@zuston
Copy link
Member

zuston commented Jul 21, 2023

I'm not sure is it necessary to bind hadoop version with image. If user don't use HDFS, this is weird

Shuffle Server must use the HDFS jars. Because some configuration use HDFS jars.

Oh, I forgot this. This should be optimized in the later PR.

@zhengchenyu
Copy link
Collaborator

I'm not sure is it necessary to bind hadoop version with image. If user don't use HDFS, this is weird

@zuston

Hadoop version is introduced by me in #896
I think we should bind hadoop version, even if we don't use HDFS. We know mapreduce, spark and tez depend on hadoop. Because the API is changed, some version of computing framework may not be compatibility with some verison of hadoop.
In production cluster, use the computing framework which is not compatibility with hadoop may produce many unexpected error.

@zuston
Copy link
Member

zuston commented Jul 21, 2023

I'm not sure is it necessary to bind hadoop version with image. If user don't use HDFS, this is weird

@zuston

Hadoop version is introduced by me in #896 I think we should bind hadoop version, even if we don't use HDFS. We know mapreduce, spark and tez depend on hadoop. Because the API is changed, some version of computing framework may not be compatibility with some verison of hadoop. In production cluster, use the computing framework which is not compatibility with hadoop may produce many unexpected error.

You are right. I have checked that all the spark distribution are all bind to hadoop.

@zhengchenyu
Copy link
Collaborator

I'm not sure is it necessary to bind hadoop version with image. If user don't use HDFS, this is weird

@zuston
Hadoop version is introduced by me in #896 I think we should bind hadoop version, even if we don't use HDFS. We know mapreduce, spark and tez depend on hadoop. Because the API is changed, some version of computing framework may not be compatibility with some verison of hadoop. In production cluster, use the computing framework which is not compatibility with hadoop may produce many unexpected error.

You are right. I have checked that all the spark distribution are all bind to hadoop.

I just copy spark! ^_^

@zuston zuston requested a review from wangao1236 July 21, 2023 13:36
Copy link
Contributor

@connorlwilkes connorlwilkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - fixes #951

@qijiale76 qijiale76 marked this pull request as ready for review August 7, 2023 11:21
Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks all.

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2023

Codecov Report

Merging #1027 (d116cfe) into master (d0f0b57) will decrease coverage by 1.02%.
Report is 41 commits behind head on master.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1027      +/-   ##
============================================
- Coverage     54.76%   53.75%   -1.02%     
+ Complexity     2526     2516      -10     
============================================
  Files           362      382      +20     
  Lines         19334    21694    +2360     
  Branches       1799     1799              
============================================
+ Hits          10588    11661    +1073     
- Misses         8116     9329    +1213     
- Partials        630      704      +74     

see 27 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jerqi jerqi merged commit 13c7143 into apache:master Aug 11, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Add the HADOOP_VERSION to the RSS Server image name by default
6 participants