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

[KYUUBI #5561] Binary distribution artifact ships database schema scripts #5589

Closed
wants to merge 2 commits into from

Conversation

lsm1
Copy link
Contributor

@lsm1 lsm1 commented Oct 31, 2023

Why are the changes needed?

close #5561

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

Was this patch authored or co-authored using generative AI tooling?

NO

build/dist Outdated
@@ -270,6 +271,9 @@ echo "Build flags: $@" >> "$DISTDIR/RELEASE"
# Copy kyuubi server jars
cp -r "$KYUUBI_HOME"/kyuubi-assembly/target/scala-$SCALA_VERSION/jars/*.jar "$DISTDIR/jars/"

# Copy kyuubi metadata sql
cp -r "$KYUUBI_HOME"/kyuubi-assembly/target/kyuubi-assembly/metastore-sql/* "$DISTDIR/metastore-sql/"
Copy link
Contributor

@cxzl25 cxzl25 Oct 31, 2023

Choose a reason for hiding this comment

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

How about to directly copy kyuubi-server/src/main/resources/sql ?

We should not need maven-assembly-plugin.

Copy link
Member

@pan3793 pan3793 Nov 1, 2023

Choose a reason for hiding this comment

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

Considering the word "metastore" is famous as an alias of HMS, we'd better not use the same word in Kyuubi terms, I suggest using the generic words to name the folder, for example, "db-schema", "db-scripts"

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2023

Codecov Report

Merging #5589 (831f06a) into master (e707bbc) will increase coverage by 0.15%.
Report is 10 commits behind head on master.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #5589      +/-   ##
============================================
+ Coverage     61.43%   61.59%   +0.15%     
  Complexity       23       23              
============================================
  Files           601      603       +2     
  Lines         34313    35670    +1357     
  Branches       4499     4869     +370     
============================================
+ Hits          21080    21970     +890     
- Misses        11098    11308     +210     
- Partials       2135     2392     +257     

see 23 files with indirect coverage changes

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

@pan3793 pan3793 added this to the v1.8.0 milestone Nov 1, 2023
@pan3793 pan3793 changed the title [KYUUBI #5561] Build dist with metastore sql [KYUUBI #5561] Binary distribution artifact ships database schema scripts Nov 1, 2023
@pan3793 pan3793 closed this in c81124b Nov 1, 2023
pan3793 pushed a commit that referenced this pull request Nov 1, 2023
…ipts

### _Why are the changes needed?_

close #5561
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [ ] [Run test](https://kyuubi.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request

### _Was this patch authored or co-authored using generative AI tooling?_

NO

Closes #5589 from lsm1/branch-kyuubi-5561.

Closes #5561

831f06a [senmiaoliu] just copy server resource sql
64b90b1 [senmiaoliu] copy metadata sql to dist

Authored-by: senmiaoliu <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
(cherry picked from commit c81124b)
Signed-off-by: Cheng Pan <[email protected]>
@pan3793
Copy link
Member

pan3793 commented Nov 1, 2023

After second thought, I decided to rename the folder to db-scripts, opened #5598 to complete that.

YesOrNo828 pushed a commit to YesOrNo828/kyuubi that referenced this pull request Nov 6, 2023
…ma scripts

### _Why are the changes needed?_

close apache#5561
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [ ] [Run test](https://kyuubi.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request

### _Was this patch authored or co-authored using generative AI tooling?_

NO

Closes apache#5589 from lsm1/branch-kyuubi-5561.

Closes apache#5561

831f06a [senmiaoliu] just copy server resource sql
64b90b1 [senmiaoliu] copy metadata sql to dist

Authored-by: senmiaoliu <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
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.

[TASK][EASY] Build dist with metastore sql
4 participants