Skip to content

ARROW-5776: [Gandiva][Crossbow] Use commit id instead of fetch head.#4738

Closed
praveenbingo wants to merge 8 commits intoapache:masterfrom
praveenbingo:ARROW-5776
Closed

ARROW-5776: [Gandiva][Crossbow] Use commit id instead of fetch head.#4738
praveenbingo wants to merge 8 commits intoapache:masterfrom
praveenbingo:ARROW-5776

Conversation

@praveenbingo
Copy link
Copy Markdown
Contributor

  • It is important to use the same commit to build the various gandiva artifacts
    on OsX/Linux etc, so that they are consistent.
  • We use the commit id to pin all of the builds to the same commit on the tree.

@praveenbingo
Copy link
Copy Markdown
Contributor Author

@ursabot crossbow package -g gandiva

@ursabot
Copy link
Copy Markdown

ursabot commented Jun 28, 2019

AMD64 Conda Crossbow (#27835) builder has been succeeded.

Revision: b6556ffdc678811a367bdd72cb50da7a0b1fce41

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-96

Task Status
gandiva-jar-osx TravisCI
gandiva-jar-trusty TravisCI

@praveenbingo
Copy link
Copy Markdown
Contributor Author

@kszucs The ursabot failure looks like a random job termination. Please take a look if this looks ok to you.

@praveenbingo
Copy link
Copy Markdown
Contributor Author

@ursabot crossbow package -g gandiva

@ursabot
Copy link
Copy Markdown

ursabot commented Jun 28, 2019

AMD64 Conda Crossbow (#27997) builder has been succeeded.

Revision: f91ecb42fe9b718e5607d0266a86fe5bd143cfa4

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-97

Task Status
gandiva-jar-osx TravisCI
gandiva-jar-trusty TravisCI

@kszucs
Copy link
Copy Markdown
Member

kszucs commented Jun 28, 2019

@praveenbingo in order to crossbow work properly with ursabot We need to use FETCH_HEAD. Ursabot builds the magic refs provided by github: refs/pull/<PR-id>/merge, see an example here. This magic reference doesn't exist until you push to the github remote.

Could You give me a little more context why it is not working for you? I'm sure We can find out a solution for both use cases.

Please don't merge yet.

@praveenbingo
Copy link
Copy Markdown
Contributor Author

@kszucs
We use crossbow to create Gandiva artifacts for both mac and linux and then we have an internal dremio job that collects those artifacts and packages it into a single maven fat jar.

We need all of the builds - Gandiva Mac, Gandiva Linux and the internal jar packager to work off the same versions in the tree to avoid inconsistencies. So we need this commit id in the cross bow file as an anchor to ensure that the three build jobs work using the same code.

Please let me know if this helps.

@wesm
Copy link
Copy Markdown
Member

wesm commented Jun 28, 2019

It seems like this synchronization issue is something that the script that runs the multiple Crossbow builds should take care of. All of the branches that you push to your Crossbow queue should be based off the same commit id

@praveenbingo
Copy link
Copy Markdown
Contributor Author

@wesm Yes I agree. And it was achieved earlier using the commit id as part of the trigger to the crossbow branches.
Is there a replacement for the same (some flag to pass perhaps?)

@praveenbingo
Copy link
Copy Markdown
Contributor Author

@kszucs Do you think we can push this in until we have an alternative?

@kszucs
Copy link
Copy Markdown
Member

kszucs commented Jun 30, 2019

We should add a flag to crossbow whether to use fetch head or the commit sha.
I wouldn't merge it until we're done with the release, but with a flag we could keep both requirements working.

@praveenbingo
Copy link
Copy Markdown
Contributor Author

@kszucs Sounds good, i will unblock by releasing temporarily from a private fork. Will get to the requested change in a couple days. Thanks!

…head

- It is important to use the same commit to build the various gandiva artifacts
  on OsX/Linux etc, so that they are consistent.
- We use the commit id to pin all of the builds to the same commit on the tree.
@praveenbingo
Copy link
Copy Markdown
Contributor Author

@ursabot crossbow package -g gandiva

@praveenbingo
Copy link
Copy Markdown
Contributor Author

@kszucs Can you please see if the current change is ok?

@ursabot
Copy link
Copy Markdown

ursabot commented Jul 26, 2019

AMD64 Conda Crossbow (#43561) builder has been succeeded.

Revision: 94c86fa

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-130

Task Status
gandiva-jar-osx TravisCI
gandiva-jar-trusty TravisCI

@praveenbingo
Copy link
Copy Markdown
Contributor Author

sample build where commit id is used https://travis-ci.org/praveenbingo/arrow-build/builds/563952605

fix style error
@pravindra
Copy link
Copy Markdown
Contributor

@kszucs can you please review this change ?

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #4738 into master will increase coverage by 0.57%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4738      +/-   ##
==========================================
+ Coverage   88.53%   89.11%   +0.57%     
==========================================
  Files         912      721     -191     
  Lines      116620   101649   -14971     
  Branches     1418        0    -1418     
==========================================
- Hits       103255    90582   -12673     
+ Misses      13003    11067    -1936     
+ Partials      362        0     -362
Impacted Files Coverage Δ
cpp/src/arrow/array-union-test.cc 100% <100%> (ø) ⬆️
cpp/src/arrow/util/rle-encoding-test.cc 96.97% <0%> (-1.16%) ⬇️
cpp/src/arrow/util/thread-pool-test.cc 97.66% <0%> (-0.94%) ⬇️
cpp/src/parquet/encoding.h 97.82% <0%> (-0.64%) ⬇️
cpp/src/parquet/column_reader.cc 88.93% <0%> (-0.55%) ⬇️
cpp/src/parquet/arrow/schema.cc 90.96% <0%> (-0.54%) ⬇️
cpp/src/parquet/encoding.cc 93.73% <0%> (-0.26%) ⬇️
cpp/src/arrow/array.h 98.29% <0%> (-0.01%) ⬇️
cpp/src/parquet/arrow/writer.cc 96.71% <0%> (ø) ⬆️
cpp/src/parquet/encoding-test.cc 100% <0%> (ø) ⬆️
... and 210 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 937bcb7...05c3888. Read the comment docs.

@kszucs
Copy link
Copy Markdown
Member

kszucs commented Jul 30, 2019

@pravindra where is $CROSSBOW_USE_COMMIT_ID set?

@praveenbingo
Copy link
Copy Markdown
Contributor Author

@kszucs it is set in Travis env variables

@pravindra
Copy link
Copy Markdown
Contributor

pravindra commented Aug 1, 2019

@kszucs this travis job picked up the env variable (CROSSBOW_USE_COMMIT_ID) and ran successfully.

https://travis-ci.org/praveenbingo/arrow-build/jobs/563952606

Please let's know if you want some change in the fix. Our (dremio) repo has been out-of-sync with arrow master for a while, and is blocked on this change.

@pravindra
Copy link
Copy Markdown
Contributor

@kszucs @wesm - i'll go ahead and merge this change tomorrow unless there are any objections.

@kszucs
Copy link
Copy Markdown
Member

kszucs commented Aug 2, 2019

@pravindra I'm fine with this change, mostly because it only affects the gandiva build. In the future we should factor out this change to the level of the submit command.

@pravindra
Copy link
Copy Markdown
Contributor

thanks @kszucs

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.

6 participants