Skip to content

Upgrade ORC to 1.6.9#11518

Merged
jihoonson merged 4 commits intoapache:masterfrom
dongjoon-hyun:orc169
Aug 1, 2021
Merged

Upgrade ORC to 1.6.9#11518
jihoonson merged 4 commits intoapache:masterfrom
dongjoon-hyun:orc169

Conversation

@dongjoon-hyun
Copy link
Copy Markdown
Member

@dongjoon-hyun dongjoon-hyun commented Jul 30, 2021

Description

This PR aims to upgrade Apache ORC from 1.5.10 to 1.6.9 and licenses.yaml correspondingly to bring the latest bug fixes and updates.

Apache ORC 1.5.10 is too old and the following versions are available. Apache ORC community highly recommends to use 1.6.9.


This PR has:

  • [v] been self-reviewed.
  • [v] added or updated version, license, or notice information in licenses.yaml

@rohangarg
Copy link
Copy Markdown
Member

@dongjoon-hyun - Thanks for the change - I wanted to ask that in the orc-extensions pom, do we have to include orc-core are a dependency separately (it is so in the current code)? I saw that orc-mapreduce has marked orc-core as its dependency so maybe we could just use that. I also checked and orc-core is only used in orc extension. wdyt?

@dongjoon-hyun
Copy link
Copy Markdown
Member Author

Hi, @rohangarg . Thank you for review.

Yes. AFAIK, the orc-core dependency is there because we want to cut the transitive dependencies of it. If we use only orc-mapreduce dependency and rely on the Maven's transitive dependency resolutions, the global dependency tree might be affected.

do we have to include orc-core are a dependency separately (it is so in the current code)?

        <dependency>
            <groupId>org.apache.orc</groupId>
            <artifactId>orc-core</artifactId>
            <version>${orc.version}</version>
            <exclusions>
                <exclusion>
                    <groupId>com.google.protobuf</groupId>
                    <artifactId>protobuf-java</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>commons-lang</groupId>
                    <artifactId>commons-lang</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>javax.xml.bind</groupId>
                    <artifactId>jaxb-api</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>org.apache.hadoop</groupId>
                    <artifactId>hadoop-hdfs</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>org.slf4j</groupId>
                    <artifactId>slf4j-api</artifactId>
                </exclusion>
            </exclusions>
        </dependency>

@dongjoon-hyun
Copy link
Copy Markdown
Member Author

dongjoon-hyun commented Jul 30, 2021

Oh, I double-checked it. It seems that the exclusion rules of orc-mapreduce includes all exclusion rules of orc-core. I'll revise this PR according to your advice, @rohangarg . Thank you again. Let's see what CIs say.

@dongjoon-hyun
Copy link
Copy Markdown
Member Author

@rohangarg . Are you sure about your comment? Druid analyze check seems to enforce the declaration.

I saw that orc-mapreduce has marked orc-core as its dependency so maybe we could just use that.

[WARNING] Used undeclared dependencies found:
[WARNING]    org.apache.orc:orc-core:jar:1.6.9:compile
[INFO] Add the following to your pom to correct the missing dependencies: 
[INFO] 
<dependency>
  <groupId>org.apache.orc</groupId>
  <artifactId>orc-core</artifactId>
  <version>1.6.9</version>
</dependency>

I'll revert the last commit because this PR already passed before the last commit.

This reverts commit 3bb219b.
@rohangarg
Copy link
Copy Markdown
Member

@dongjoon-hyun : Yes, you are right. Apologies, I missed that dependency analysis in druid fails compilation if we're not declaring any directly used dependency (the default maven behavior gives a warning). Maybe that was the original reason to keep it explicit :) I had missed the -DfailOnWarning=true flag during local check so the compile had passed for me.
Also, I did a quick lookup and the observation you had wrt to the transitive dependencies is correct - the maven page states that Exclusions work on the entire dependency graph below the point where they are declared. (https://maven.apache.org/guides/introduction/introduction-to-optional-and-excludes-dependencies.html)

You updated change LGTM - thanks for the tryout and changes!

@dongjoon-hyun
Copy link
Copy Markdown
Member Author

Could you review this PR when you have a time, @jihoonson ?
This PR already passed at 9698b7d and now I reverted this to that commit.
Among, the latest two failures,

  1. The dependency change is reverted
  2. The IOException issue will be fixed Fix Travis CI for master #11522 .

@dongjoon-hyun
Copy link
Copy Markdown
Member Author

:) I also learned it at this time. Thank you for you reviews and helps, @rohangarg .

@jihoonson
Copy link
Copy Markdown
Contributor

@dongjoon-hyun sure, I will have a look soon.

@dongjoon-hyun
Copy link
Copy Markdown
Member Author

The one failure looks irrelevant to this one because the last commit (2feb9a3) is identical to the this commit (9698b7d) which passed all tested.

[ERROR] Failures: 
[ERROR]   ITKafkaIndexingServiceNonTransactionalParallelizedTest.testKafkaIndexDataWithWithAutoscaler:62->AbstractStreamIndexingTest.doTestIndexDataWithAutoscaler:342 » ISE
[ERROR] Tests run: 4, Failures: 1, Errors: 0, Skipped: 0

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson 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 @dongjoon-hyun!

@jihoonson jihoonson merged commit dbed442 into apache:master Aug 1, 2021
@dongjoon-hyun
Copy link
Copy Markdown
Member Author

Thank you so much, @jihoonson !

@dongjoon-hyun dongjoon-hyun deleted the orc169 branch August 2, 2021 00:52
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants