-
Notifications
You must be signed in to change notification settings - Fork 299
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
[#4759] feat(auth-ranger): Use Spark verify Ranger authorization Hive #4948
base: main
Are you sure you want to change the base?
Conversation
bb0092c
to
d399416
Compare
...e-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/TestIcebergTable.java
Outdated
Show resolved
Hide resolved
72789d7
to
dba750d
Compare
dc5a9e9
to
ee98f63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FANNG1 Thank you for your help me to fix log4j
issue.
Please help me review this PR, thanks.
...e-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/TestIcebergTable.java
Outdated
Show resolved
Hide resolved
|
||
testImplementation(platform("org.junit:junit-bom:5.9.1")) | ||
testImplementation("org.junit.jupiter:junit-jupiter") | ||
val testShadowJar by tasks.registering(ShadowJar::class) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after using ShadowJar
, the integration test common jar will include almost all dependences, is it expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The integration-test-comman
ShadowJar main use to running MiniGravitino
service.
We can expend a single MiniGravitino
module in the Gravitino project in the future.
@mchades What do you think?
@@ -260,6 +260,29 @@ private void initializeWebAppServletContextHandler() { | |||
servletContextHandler = new WebAppContext(); | |||
|
|||
boolean isUnitTest = System.getenv("GRAVITINO_TEST") != null; | |||
if (isUnitTest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about this change, you relocate the jetty package not exclude it, why couldn't find the jetty class?
@@ -299,7 +322,7 @@ private void initializeWebAppServletContextHandler() { | |||
if (warFile.isDirectory()) { | |||
// Development mode, read from FS | |||
servletContextHandler.setResourceBase(warFile.getPath()); | |||
servletContextHandler.setContextPath("/"); | |||
servletContextHandler.addServlet(DefaultServlet.class, "/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason changing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use a web.xml
to initial the servletContextHandler. So need change this line.
testImplementation(project(":server")) | ||
testImplementation(project(":server-common")) | ||
testImplementation(project(":core")) { | ||
exclude(group = "org.rocksdb", module = "rocksdbjni") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason exclude rocksdb
? since this is used for test only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rocksdb-xxx.jar
have 47MB size, let Shadow jar package very big, and currently we doesn't support rocksdb storage. So I exclude it.
@@ -73,7 +73,7 @@ public static void setUp() throws Exception { | |||
|
|||
mySQLCatalog = | |||
new TestCatalog() | |||
.withCatalogConf(ImmutableMap.of(Catalog.AUTHORIZATION_PROVIDER, "mysql")) | |||
.withCatalogConf(ImmutableMap.of(Catalog.AUTHORIZATION_PROVIDER, "mysqlTest")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use a static variable? since it's shared by different classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not other place needs to use this static variable.
What changes were proposed in this pull request?
Why are the changes needed?
Fix: #4759
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
CI Passed.