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

[AMORO-2862] fix bug: A null pointer error occurs when using S3 as Storage when using the kyuubi terminal. #2863

Merged
merged 8 commits into from
May 30, 2024

Conversation

upczsh
Copy link
Contributor

@upczsh upczsh commented May 24, 2024

Why are the changes needed?

Close #2862.

Brief change log

  • A null pointer error occurs when using S3 as Storage when using the kyuubi terminal.Determine in the kyuubi terminal. If the catalog type is s3 type, then do not add hadoopusername to avoid null pointer errors.

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
    Before repair:
    image

after repair:
image

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

zhaoshuaihua added 3 commits May 24, 2024 19:15
…ng S3 as Storage when using the kyuubi terminal.
…ng S3 as Storage when using the kyuubi terminal.
…ng S3 as Storage when using the kyuubi terminal.
@github-actions github-actions bot added the module:core Core module label May 24, 2024
Copy link
Contributor

@zhoujinsong zhoujinsong left a comment

Choose a reason for hiding this comment

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

@upczsh Thanks for reporting this bug and trying to fix it.

I left some comments, PTAL.

@@ -124,8 +125,11 @@ public TerminalSession create(TableMetaStore metaStore, Configurations configura
Properties properties = new Properties();

if (!metaStore.isKerberosAuthMethod()) {
properties.put(JdbcConnectionParams.AUTH_USER, metaStore.getHadoopUsername());
Copy link
Contributor

Choose a reason for hiding this comment

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

How about checking if the value is null before putting it into properties and configuration?
I think it will be simpler if we fix it this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestion, I have made the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use one condition to fix this, WDYT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use one condition to fix this, WDYT

Okay, I've merged it into one.thinks.

@github-actions github-actions bot removed the module:core Core module label May 28, 2024
@upczsh upczsh requested review from zhoujinsong and XBaith May 28, 2024 04:06
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 32.76%. Comparing base (a2753b6) to head (9fa709b).
Report is 14 commits behind head on master.

Files Patch % Lines
.../terminal/kyuubi/KyuubiTerminalSessionFactory.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2863      +/-   ##
============================================
- Coverage     35.95%   32.76%   -3.20%     
- Complexity      383     3820    +3437     
============================================
  Files            45      558     +513     
  Lines          4700    46295   +41595     
  Branches        513     6175    +5662     
============================================
+ Hits           1690    15169   +13479     
- Misses         2842    29931   +27089     
- Partials        168     1195    +1027     
Flag Coverage Δ
core 32.76% <0.00%> (?)
trino ?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@baiyangtx baiyangtx merged commit ad46f97 into apache:master May 30, 2024
4 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.

[Bug]: A null pointer error occurs when using S3 as Storage when using the kyuubi terminal.
5 participants