ayushtkn commented on code in PR #5404:
URL: https://github.com/apache/hive/pull/5404#discussion_r1976697825
##########
Jenkinsfile:
##########
@@ -290,12 +291,14 @@ fi
stage('init-metastore') {
withEnv(["dbType=$dbType"]) {
sh '''#!/bin/bash -e
+ sw java 17 && . /etc/profile.d/java.sh
set -x
echo 127.0.0.1 dev_$dbType | sudo tee -a /etc/hosts
. /etc/profile.d/confs.sh
sw hive-dev $PWD
export DOCKER_NETWORK=host
export DBNAME=metastore
+export HADOOP_CLIENT_OPTS="--add-opens java.base/java.net=ALL-UNNAMED"
Review Comment:
Is it required in the `Jenkinsfile`, hive should add these arguments on its
own via env files or so
##########
itests/hive-unit/src/test/java/org/apache/hive/service/cli/thrift/TestThriftHttpCLIServiceFeatures.java:
##########
@@ -351,8 +351,8 @@ private void verifyForwardedHeaders(ArrayList<String>
headerIPs, String cmd) thr
.forClass(HiveAuthzContext.class);
verify(mockedAuthorizer).checkPrivileges(any(HiveOperationType.class),
- Matchers.anyListOf(HivePrivilegeObject.class),
- Matchers.anyListOf(HivePrivilegeObject.class),
contextCapturer.capture());
+ anyList(),
+ anyList(), contextCapturer.capture());
Review Comment:
The assertion changes I believe now. Earlier it was expecting a list of type
``HivePrivilegeObject`` now any list will give it as pass
##########
ql/src/test/results/clientpositive/llap/mm_dp.q.out:
##########
@@ -4370,360 +4370,360 @@ POSTHOOK: Output: hdfs://### HDFS PATH ###
437 458 2 val_437
437 458 20 val_437
437 459 20 val_437
-438 456 1 val_438
-438 456 10 val_438
-438 456 27 val_438
-438 457 19 val_438
-438 457 24 val_438
-438 457 25 val_438
-438 458 5 val_438
-438 458 13 val_438
-438 458 19 val_438
-438 458 24 val_438
-438 458 25 val_438
+438 456 16 val_438
+438 456 20 val_438
+438 456 20 val_438
+438 457 6 val_438
+438 457 6 val_438
+438 457 28 val_438
+438 458 6 val_438
+438 458 6 val_438
+438 458 6 val_438
+438 458 16 val_438
438 458 28 val_438
-438 459 19 val_438
-438 459 24 val_438
-438 459 25 val_438
-439 456 16 val_439
-439 456 28 val_439
-439 457 7 val_439
-439 457 17 val_439
-439 458 7 val_439
-439 458 7 val_439
-439 458 11 val_439
-439 458 17 val_439
-439 459 7 val_439
-439 459 17 val_439
-443 456 6 val_443
+438 458 28 val_438
+438 459 6 val_438
+438 459 6 val_438
+438 459 28 val_438
+439 456 19 val_439
+439 456 27 val_439
+439 457 4 val_439
+439 457 27 val_439
+439 458 4 val_439
+439 458 4 val_439
+439 458 27 val_439
+439 458 27 val_439
+439 459 4 val_439
+439 459 27 val_439
+443 456 23 val_443
443 457 24 val_443
-443 458 5 val_443
+443 458 24 val_443
443 458 24 val_443
443 459 24 val_443
-444 456 25 val_444
-444 457 15 val_444
-444 458 14 val_444
-444 458 15 val_444
-444 459 15 val_444
-446 456 6 val_446
-446 457 0 val_446
-446 458 0 val_446
-446 458 5 val_446
-446 459 0 val_446
-448 456 23 val_448
-448 457 10 val_448
-448 458 10 val_448
-448 458 24 val_448
-448 459 10 val_448
-449 456 19 val_449
-449 457 20 val_449
-449 458 1 val_449
-449 458 20 val_449
-449 459 20 val_449
-452 456 16 val_452
-452 457 6 val_452
-452 458 6 val_452
-452 458 28 val_452
-452 459 6 val_452
-453 456 28 val_453
-453 457 16 val_453
-453 458 16 val_453
-453 458 20 val_453
-453 459 16 val_453
-454 456 1 val_454
+444 456 19 val_444
+444 457 4 val_444
+444 458 4 val_444
+444 458 4 val_444
+444 459 4 val_444
+446 456 23 val_446
+446 457 9 val_446
+446 458 9 val_446
+446 458 24 val_446
+446 459 9 val_446
+448 456 4 val_448
+448 457 27 val_448
+448 458 1 val_448
+448 458 27 val_448
+448 459 27 val_448
+449 456 13 val_449
+449 457 9 val_449
+449 458 9 val_449
+449 458 28 val_449
+449 459 9 val_449
+452 456 23 val_452
+452 457 23 val_452
+452 458 23 val_452
+452 458 23 val_452
+452 459 23 val_452
+453 456 23 val_453
+453 457 2 val_453
+453 458 2 val_453
+453 458 9 val_453
+453 459 2 val_453
+454 456 7 val_454
454 456 10 val_454
-454 456 16 val_454
-454 457 6 val_454
-454 457 19 val_454
-454 457 24 val_454
-454 458 1 val_454
-454 458 6 val_454
+454 456 20 val_454
+454 457 11 val_454
+454 457 13 val_454
+454 457 23 val_454
+454 458 5 val_454
+454 458 10 val_454
+454 458 11 val_454
454 458 13 val_454
-454 458 14 val_454
-454 458 19 val_454
-454 458 24 val_454
-454 459 6 val_454
-454 459 19 val_454
-454 459 24 val_454
-455 456 6 val_455
-455 457 17 val_455
-455 458 11 val_455
-455 458 17 val_455
-455 459 17 val_455
+454 458 23 val_454
+454 458 23 val_454
+454 459 11 val_454
+454 459 13 val_454
+454 459 23 val_454
+455 456 9 val_455
+455 457 28 val_455
+455 458 16 val_455
+455 458 28 val_455
+455 459 28 val_455
457 456 1 val_457
457 457 7 val_457
457 458 2 val_457
457 458 7 val_457
457 459 7 val_457
-458 456 7 val_458
-458 456 10 val_458
+458 456 9 val_458
+458 456 16 val_458
458 457 4 val_458
-458 457 10 val_458
+458 457 19 val_458
458 458 4 val_458
-458 458 10 val_458
-458 458 10 val_458
+458 458 6 val_458
+458 458 13 val_458
458 458 19 val_458
458 459 4 val_458
-458 459 10 val_458
-459 456 10 val_459
+458 459 19 val_458
459 456 20 val_459
+459 456 28 val_459
459 457 1 val_459
-459 457 10 val_459
+459 457 20 val_459
459 458 1 val_459
-459 458 10 val_459
-459 458 10 val_459
+459 458 14 val_459
+459 458 20 val_459
459 458 20 val_459
459 459 1 val_459
-459 459 10 val_459
-460 456 23 val_460
-460 457 28 val_460
-460 458 28 val_460
-460 458 28 val_460
-460 459 28 val_460
-462 456 11 val_462
-462 456 27 val_462
-462 457 3 val_462
+459 459 20 val_459
+460 456 3 val_460
+460 457 20 val_460
+460 458 20 val_460
+460 458 20 val_460
+460 459 20 val_460
+462 456 9 val_462
+462 456 24 val_462
462 457 5 val_462
-462 458 3 val_462
+462 457 9 val_462
462 458 5 val_462
-462 458 14 val_462
+462 458 9 val_462
+462 458 9 val_462
462 458 16 val_462
-462 459 3 val_462
462 459 5 val_462
-463 456 16 val_463
-463 456 24 val_463
-463 457 4 val_463
-463 457 20 val_463
-463 458 4 val_463
-463 458 7 val_463
+462 459 9 val_462
+463 456 5 val_463
+463 456 23 val_463
+463 457 6 val_463
+463 457 9 val_463
+463 458 6 val_463
463 458 9 val_463
-463 458 20 val_463
-463 459 4 val_463
-463 459 20 val_463
-466 456 9 val_466
-466 456 16 val_466
-466 456 19 val_466
-466 457 5 val_466
-466 457 11 val_466
-466 457 23 val_466
-466 458 2 val_466
+463 458 25 val_463
+463 458 28 val_463
+463 459 6 val_463
+463 459 9 val_463
+466 456 7 val_466
+466 456 10 val_466
+466 456 10 val_466
+466 457 1 val_466
+466 457 13 val_466
+466 457 20 val_466
+466 458 1 val_466
466 458 5 val_466
-466 458 7 val_466
-466 458 11 val_466
-466 458 23 val_466
-466 458 23 val_466
-466 459 5 val_466
-466 459 11 val_466
-466 459 23 val_466
-467 456 13 val_467
-467 457 23 val_467
-467 458 4 val_467
-467 458 23 val_467
-467 459 23 val_467
-468 456 7 val_468
-468 456 7 val_468
+466 458 10 val_466
+466 458 13 val_466
+466 458 19 val_466
+466 458 20 val_466
+466 459 1 val_466
+466 459 13 val_466
+466 459 20 val_466
+467 456 27 val_467
+467 457 7 val_467
+467 458 7 val_467
+467 458 28 val_467
+467 459 7 val_467
+468 456 1 val_468
+468 456 14 val_468
468 456 19 val_468
-468 456 24 val_468
-468 457 7 val_468
-468 457 10 val_468
+468 456 20 val_468
+468 457 5 val_468
+468 457 6 val_468
468 457 10 val_468
-468 457 14 val_468
-468 458 7 val_468
-468 458 10 val_468
-468 458 10 val_468
-468 458 10 val_468
+468 457 16 val_468
+468 458 3 val_468
+468 458 5 val_468
+468 458 6 val_468
468 458 10 val_468
468 458 14 val_468
-468 458 14 val_468
-468 458 17 val_468
-468 459 7 val_468
-468 459 10 val_468
+468 458 16 val_468
+468 458 16 val_468
+468 458 27 val_468
+468 459 5 val_468
+468 459 6 val_468
468 459 10 val_468
-468 459 14 val_468
-469 456 1 val_469
+468 459 16 val_468
+469 456 13 val_469
469 456 13 val_469
469 456 14 val_469
-469 456 19 val_469
469 456 20 val_469
+469 456 28 val_469
+469 457 1 val_469
469 457 1 val_469
469 457 1 val_469
-469 457 9 val_469
-469 457 24 val_469
+469 457 11 val_469
469 457 27 val_469
469 458 1 val_469
469 458 1 val_469
-469 458 9 val_469
-469 458 14 val_469
+469 458 1 val_469
+469 458 6 val_469
+469 458 11 val_469
469 458 16 val_469
-469 458 24 val_469
+469 458 23 val_469
469 458 24 val_469
469 458 27 val_469
469 458 28 val_469
-469 458 28 val_469
469 459 1 val_469
469 459 1 val_469
-469 459 9 val_469
-469 459 24 val_469
+469 459 1 val_469
+469 459 11 val_469
469 459 27 val_469
470 456 27 val_470
470 457 1 val_470
470 458 1 val_470
470 458 1 val_470
470 459 1 val_470
-472 456 6 val_472
-472 457 14 val_472
+472 456 16 val_472
+472 457 11 val_472
+472 458 5 val_472
472 458 11 val_472
-472 458 14 val_472
-472 459 14 val_472
-475 456 7 val_475
-475 457 6 val_475
-475 458 6 val_475
-475 458 6 val_475
-475 459 6 val_475
-477 456 14 val_477
-477 457 7 val_477
-477 458 2 val_477
-477 458 7 val_477
-477 459 7 val_477
+472 459 11 val_472
+475 456 1 val_475
+475 457 9 val_475
+475 458 9 val_475
+475 458 9 val_475
+475 459 9 val_475
+477 456 24 val_477
+477 457 5 val_477
+477 458 5 val_477
+477 458 10 val_477
+477 459 5 val_477
478 456 9 val_478
-478 456 13 val_478
-478 457 4 val_478
-478 457 9 val_478
-478 458 4 val_478
-478 458 9 val_478
-478 458 9 val_478
+478 456 10 val_478
+478 457 10 val_478
+478 457 19 val_478
+478 458 10 val_478
+478 458 10 val_478
+478 458 13 val_478
478 458 19 val_478
-478 459 4 val_478
-478 459 9 val_478
-479 456 20 val_479
-479 457 17 val_479
-479 458 1 val_479
-479 458 17 val_479
-479 459 17 val_479
-480 456 14 val_480
-480 456 24 val_480
-480 456 27 val_480
-480 457 9 val_480
-480 457 16 val_480
-480 457 27 val_480
-480 458 9 val_480
+478 459 10 val_478
+478 459 19 val_478
+479 456 7 val_479
+479 457 7 val_479
+479 458 7 val_479
+479 458 7 val_479
+479 459 7 val_479
+480 456 1 val_480
+480 456 1 val_480
+480 456 20 val_480
+480 457 2 val_480
+480 457 11 val_480
+480 457 20 val_480
+480 458 2 val_480
480 458 9 val_480
-480 458 16 val_480
-480 458 27 val_480
-480 458 27 val_480
-480 458 27 val_480
-480 459 9 val_480
-480 459 16 val_480
-480 459 27 val_480
+480 458 11 val_480
+480 458 11 val_480
+480 458 20 val_480
+480 458 20 val_480
+480 459 2 val_480
+480 459 11 val_480
+480 459 20 val_480
481 456 28 val_481
481 457 5 val_481
481 458 5 val_481
481 458 10 val_481
481 459 5 val_481
-482 456 9 val_482
-482 457 9 val_482
-482 458 9 val_482
-482 458 9 val_482
-482 459 9 val_482
-483 456 5 val_483
-483 457 3 val_483
-483 458 3 val_483
-483 458 25 val_483
-483 459 3 val_483
+482 456 14 val_482
+482 457 2 val_482
+482 458 2 val_482
+482 458 27 val_482
+482 459 2 val_482
+483 456 4 val_483
+483 457 20 val_483
+483 458 1 val_483
+483 458 20 val_483
+483 459 20 val_483
484 456 10 val_484
484 457 23 val_484
484 458 13 val_484
484 458 23 val_484
484 459 23 val_484
-485 456 7 val_485
-485 457 28 val_485
-485 458 1 val_485
-485 458 28 val_485
-485 459 28 val_485
-487 456 10 val_487
-487 457 10 val_487
-487 458 10 val_487
-487 458 10 val_487
-487 459 10 val_487
-489 456 4 val_489
-489 456 9 val_489
+485 456 13 val_485
+485 457 4 val_485
+485 458 4 val_485
+485 458 4 val_485
+485 459 4 val_485
+487 456 28 val_487
+487 457 9 val_487
+487 458 9 val_487
+487 458 14 val_487
+487 459 9 val_487
+489 456 2 val_489
489 456 10 val_489
-489 456 14 val_489
+489 456 24 val_489
+489 456 27 val_489
+489 457 6 val_489
489 457 9 val_489
-489 457 9 val_489
-489 457 19 val_489
+489 457 13 val_489
489 457 27 val_489
-489 458 1 val_489
-489 458 9 val_489
+489 458 6 val_489
489 458 9 val_489
+489 458 10 val_489
489 458 13 val_489
-489 458 19 val_489
+489 458 20 val_489
489 458 24 val_489
489 458 27 val_489
489 458 27 val_489
+489 459 6 val_489
489 459 9 val_489
-489 459 9 val_489
-489 459 19 val_489
+489 459 13 val_489
489 459 27 val_489
-490 456 3 val_490
-490 457 15 val_490
-490 458 1 val_490
-490 458 15 val_490
-490 459 15 val_490
-491 456 16 val_491
-491 457 1 val_491
+490 456 9 val_490
+490 457 4 val_490
+490 458 4 val_490
+490 458 10 val_490
+490 459 4 val_490
+491 456 2 val_491
+491 457 4 val_491
491 458 1 val_491
-491 458 6 val_491
-491 459 1 val_491
-492 456 6 val_492
-492 456 13 val_492
-492 457 14 val_492
-492 457 15 val_492
-492 458 14 val_492
-492 458 15 val_492
-492 458 17 val_492
-492 458 23 val_492
-492 459 14 val_492
-492 459 15 val_492
-493 456 4 val_493
-493 457 23 val_493
-493 458 23 val_493
-493 458 23 val_493
-493 459 23 val_493
-494 456 10 val_494
-494 457 13 val_494
-494 458 13 val_494
-494 458 13 val_494
-494 459 13 val_494
+491 458 4 val_491
+491 459 4 val_491
+492 456 10 val_492
+492 456 20 val_492
+492 457 7 val_492
+492 457 24 val_492
+492 458 3 val_492
+492 458 4 val_492
+492 458 7 val_492
+492 458 24 val_492
+492 459 7 val_492
+492 459 24 val_492
+493 456 16 val_493
+493 457 7 val_493
+493 458 7 val_493
+493 458 7 val_493
+493 459 7 val_493
+494 456 20 val_494
+494 457 28 val_494
+494 458 16 val_494
+494 458 28 val_494
+494 459 28 val_494
495 456 27 val_495
495 457 25 val_495
495 458 7 val_495
495 458 25 val_495
495 459 25 val_495
-496 456 28 val_496
-496 457 2 val_496
-496 458 2 val_496
-496 458 4 val_496
-496 459 2 val_496
-497 456 28 val_497
-497 457 4 val_497
-497 458 4 val_497
-497 458 20 val_497
-497 459 4 val_497
-498 456 3 val_498
-498 456 10 val_498
+496 456 1 val_496
+496 457 5 val_496
+496 458 5 val_496
+496 458 10 val_496
+496 459 5 val_496
+497 456 1 val_497
+497 457 14 val_497
+497 458 14 val_497
+497 458 24 val_497
+497 459 14 val_497
+498 456 2 val_498
+498 456 28 val_498
498 456 28 val_498
-498 457 10 val_498
-498 457 14 val_498
+498 457 4 val_498
+498 457 11 val_498
498 457 28 val_498
-498 458 10 val_498
-498 458 10 val_498
-498 458 14 val_498
-498 458 15 val_498
+498 458 4 val_498
+498 458 11 val_498
+498 458 20 val_498
+498 458 20 val_498
498 458 20 val_498
498 458 28 val_498
-498 459 10 val_498
-498 459 14 val_498
+498 459 4 val_498
+498 459 11 val_498
Review Comment:
why is this entire file changing? this looks like query output change, which
we should try our best to avoid
##########
common/src/java/org/apache/hadoop/hive/common/CompressionUtils.java:
##########
@@ -169,30 +169,30 @@ public static List<File> unTar(final String
inputFileName, final String outputDi
// no sub-directories
continue;
}
- LOG.debug(String.format("Attempting to write output directory %s.",
- outputFile.getAbsolutePath()));
+ LOG.debug("Attempting to write output directory %s.".formatted(
+ outputFile.getAbsolutePath()));
if (!outputFile.exists()) {
- LOG.debug(String.format("Attempting to create output directory
%s.",
- outputFile.getAbsolutePath()));
+ LOG.debug("Attempting to create output directory %s.".formatted(
+ outputFile.getAbsolutePath()));
if (!outputFile.mkdirs()) {
- throw new IllegalStateException(String.format("Couldn't create
directory %s.",
- outputFile.getAbsolutePath()));
+ throw new IllegalStateException("Couldn't create directory
%s.".formatted(
+ outputFile.getAbsolutePath()));
}
}
} else {
final OutputStream outputFileStream;
if (flatten) {
File flatOutputFile = new File(outputDir, outputFile.getName());
- LOG.debug(String.format("Creating flat output file %s.",
flatOutputFile.getAbsolutePath()));
+ LOG.debug("Creating flat output file
%s.".formatted(flatOutputFile.getAbsolutePath()));
outputFileStream = new FileOutputStream(flatOutputFile);
} else if (!outputFile.getParentFile().exists()) {
- LOG.debug(String.format("Attempting to create output directory
%s.",
- outputFile.getParentFile().getAbsoluteFile()));
+ LOG.debug("Attempting to create output directory %s.".formatted(
+ outputFile.getParentFile().getAbsoluteFile()));
if (!outputFile.getParentFile().getAbsoluteFile().mkdirs()) {
- throw new IllegalStateException(String.format("Couldn't create
directory %s.",
- outputFile.getParentFile().getAbsolutePath()));
+ throw new IllegalStateException("Couldn't create directory
%s.".formatted(
+ outputFile.getParentFile().getAbsolutePath()));
}
- LOG.debug(String.format("Creating output file %s.",
outputFile.getAbsolutePath()));
+ LOG.debug("Creating output file
%s.".formatted(outputFile.getAbsolutePath()));
Review Comment:
I think rather than using `formatted`, we can directly use log4 format {}
##########
common/pom.xml:
##########
@@ -453,6 +476,14 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <configuration>
+ <source>17</source>
+ <target>17</target>
+ </configuration>
+ </plugin>
Review Comment:
Why do we need to define this again, it should be inherrited from the parent
right? moreover it should use ``maven.compiler.source`` and
``maven.compiler.target``
##########
common/src/test/org/apache/hadoop/hive/conf/TestHiveConfVarsValidate.java:
##########
@@ -69,8 +69,10 @@ public static Collection<Object[]> generateParameters() {
list.add(new Object[] { HIVE_DATETIME_RESOLVER_STYLE, "smart", null});
list.add(new Object[] { HIVE_DATETIME_RESOLVER_STYLE, "strict", null});
list.add(new Object[] { HIVE_DATETIME_RESOLVER_STYLE, "lenient", null});
- list.add(new Object[] { HIVE_DATETIME_RESOLVER_STYLE, "OTHER", "Invalid
value.. expects one of [smart, strict, " +
- "lenient]" });
+ list.add(new Object[] { HIVE_DATETIME_RESOLVER_STYLE, "OTHER", """
+ Invalid value.. expects one of [smart, strict, \
+ lenient]\
Review Comment:
code ain't formatted
##########
common/src/java/org/apache/hadoop/hive/common/CopyOnFirstWriteProperties.java:
##########
@@ -361,6 +361,7 @@ public void setInterned(Properties p) {
public CopyOnFirstWriteProperties() {
}
+ @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "HIVE-23613:
intended_TO_DO")
Review Comment:
How did this surface in scope of JDK upgrade?
##########
pom.xml:
##########
@@ -184,8 +185,8 @@
<opencsv.version>5.9</opencsv.version>
<orc.version>1.9.4</orc.version>
<otel.version>1.42.0</otel.version>
- <mockito-core.version>3.4.4</mockito-core.version>
- <mockito-inline.version>4.11.0</mockito-inline.version>
+ <mockito-core.version>5.0.0</mockito-core.version>
+ <mockito-inline.version>5.0.0</mockito-inline.version>
Review Comment:
should we move to `5.15.2`?
##########
iceberg/pom.xml:
##########
@@ -33,8 +33,7 @@
<iceberg.kryo.version>5.5.0</iceberg.kryo.version>
<iceberg.checkstyle.plugin.version>3.5.0</iceberg.checkstyle.plugin.version>
<spotless.maven.plugin.version>2.5.0</spotless.maven.plugin.version>
-
<google.errorprone.javac.version>9+181-r4173-1</google.errorprone.javac.version>
- <google.errorprone.version>2.5.1</google.errorprone.version>
+ <google.errorprone.version>2.29.2</google.errorprone.version>
Review Comment:
I think we have 2.36 now, If it works we should shoot for that
##########
shims/common/src/main/java/org/apache/hadoop/hive/thrift/TFilterTransport.java:
##########
@@ -29,6 +30,7 @@
public class TFilterTransport extends TTransport {
protected final TTransport wrapped;
+ @SuppressFBWarnings(value="EI_EXPOSE_REP2", justification = "HIVE-23613:
intended_TO_DO")
Review Comment:
Seems a lot of FB warnings popping up, why are they coming with JDK version
upgrade. rather than hacking it with these changes in so many files. I think we
should disable the FB check & address/fix them in a followup
##########
data/conf/hive-site.xml:
##########
@@ -130,6 +130,21 @@
<description></description>
</property>
+<property>
+ <name>yarn.app.mapreduce.am.command-opts</name>
+ <value> --add-opens java.base/java.net=ALL-UNNAMED --add-opens
java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED --add-opens
java.base/java.util.concurrent.atomic=ALL-UNNAMED
--add-opens=java.base/java.util.regex=ALL-UNNAMED </value>
+</property>
+
+<property>
+ <name>mapreduce.map.java.opts</name>
+ <value> --add-opens java.base/java.net=ALL-UNNAMED --add-opens
java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED --add-opens
java.base/java.util.concurrent.atomic=ALL-UNNAMED </value>
+</property>
+
+<property>
+ <name>mapreduce.reduce.java.opts</name>
+ <value> --add-opens java.base/java.net=ALL-UNNAMED --add-opens
java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED --add-opens
java.base/java.util.concurrent.atomic=ALL-UNNAMED </value>
+</property>
+
Review Comment:
These seems to be used at multiple places we should extract these values.
Moreover how will these things pushed in case of normal deployment.
We expect users to explicitly put these to make things work? I think that
would be an incompatible changes & not very user friendly
##########
packaging/src/docker/Dockerfile:
##########
@@ -68,15 +68,16 @@ COPY --from=env /opt/apache-tez-$TEZ_VERSION-bin /opt/tez
# Install dependencies
RUN set -ex; \
- apt-get update; \
- apt-get -y install procps; \
+ microdnf update -y; \
+ microdnf -y install procps; \
rm -rf /var/lib/apt/lists/*
# Set necessary environment variables.
ENV HADOOP_HOME=/opt/hadoop \
HIVE_HOME=/opt/hive \
TEZ_HOME=/opt/tez \
- HIVE_VER=$HIVE_VERSION
+ HIVE_VER=$HIVE_VERSION \
+ HADOOP_CLIENT_OPTS=" --add-opens java.base/java.net=ALL-UNNAMED
--add-opens java.base/java.lang=ALL-UNNAMED --add-opens
java.base/java.nio=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED
--add-opens java.base/java.util.concurrent.atomic=ALL-UNNAMED"
Review Comment:
I am not sure if this sound like a good approach, that we need to explicitly
set ``HADOOP_CLIENT_OPTS``, hive I believe should take care.
Why putting this up in beeline.sh or hms.sh or hs2.sh, doesn't suffice?
##########
common/src/java/org/apache/hadoop/hive/conf/valcoersion/JavaIOTmpdirVariableCoercion.java:
##########
@@ -50,7 +50,7 @@ private String coerce(String originalValue) {
Path absolutePath = FileUtils.makeAbsolute(LOCAL_FILE_SYSTEM,
originalPath);
return absolutePath.toString();
} catch (IOException exception) {
- LOG.warn(String.format("Unable to resolve 'java.io.tmpdir' for absolute
path '%s'", originalValue));
+ LOG.warn("Unable to resolve 'java.io.tmpdir' for absolute path
'%s'".formatted(originalValue));
Review Comment:
use {} instead
##########
pom.xml:
##########
@@ -1894,6 +1910,29 @@
<artifactId>jamon-maven-plugin</artifactId>
<version>${jamon.plugin.version}</version>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-javadoc-plugin</artifactId>
+ <configuration>
+ <doclint>none</doclint>
+ <useStandardDocletOptions>false</useStandardDocletOptions>
+ <additionalDependencies>
+ <additionalDependency>
+ <groupId>io.dropwizard.metrics</groupId>
+ <artifactId>metrics-core</artifactId>
+ <version>3.1.0</version>
Review Comment:
extract as variable, can be 4.2.30
##########
common/src/java/org/apache/hadoop/hive/common/FileUtils.java:
##########
@@ -1007,8 +1007,10 @@ public static boolean rename(FileSystem fs, Path
sourcePath,
// If destPath directory exists, rename call will move the sourcePath
// into destPath without failing. So check it before renaming.
if (fs.exists(destPath)) {
- throw new IOException("Cannot rename the source path. The destination "
- + "path already exists.");
+ throw new IOException("""
+ Cannot rename the source path. The destination \
+ path already exists.\
+ """);
Review Comment:
Why?
##########
pom.xml:
##########
@@ -171,7 +172,7 @@
<kafka.version>2.5.0</kafka.version>
<kryo.version>5.5.0</kryo.version>
<reflectasm.version>1.11.9</reflectasm.version>
- <kudu.version>1.12.0</kudu.version>
+ <kudu.version>1.17.0</kudu.version>
Review Comment:
`1.17.1`
##########
data/conf/iceberg/tez/hive-site.xml:
##########
@@ -250,7 +250,7 @@
<property>
<name>hive.tez.java.opts</name>
- <value> -Dlog4j.configurationFile=tez-container-log4j2.properties
-Dtez.container.log.level=INFO -Dtez.container.root.logger=CLA </value>
+ <value> -Dlog4j.configurationFile=tez-container-log4j2.properties
-Dtez.container.log.level=INFO -Dtez.container.root.logger=CLA
--add-opens=java.base/java.net=ALL-UNNAMED
--add-opens=java.base/java.util=ALL-UNNAMED
--add-opens=java.base/java.util.concurrent=ALL-UNNAMED
--add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED
--add-opens=java.base/java.lang=ALL-UNNAMED
--add-opens=java.base/java.util.regex=ALL-UNNAMED
--add-opens=java.base/java.io=ALL-UNNAMED </value>
</property>
Review Comment:
We should find a way to pass these via code, else any users puts up his own
java.opts and the cluster crashes
##########
pom.xml:
##########
@@ -1894,6 +1910,29 @@
<artifactId>jamon-maven-plugin</artifactId>
<version>${jamon.plugin.version}</version>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-javadoc-plugin</artifactId>
Review Comment:
Why is this need now?
##########
llap-server/pom.xml:
##########
@@ -183,6 +183,12 @@
<dependency>
<groupId>org.apache.orc</groupId>
<artifactId>orc-core</artifactId>
+ <exclusions>
+ <exclusion>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-client-api</artifactId>
+ </exclusion>
+ </exclusions>
</dependency>
Review Comment:
why do you need to exclude, does it not lead to any runtime issues? orc-core
would be expecting some classes from `hadoop-client-api`
##########
ql/src/test/resources/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFDateFormatEvaluate.csv:
##########
@@ -15,7 +15,7 @@ Input datetime;Input pattern;Local timezone;Formatter
type;Expected output
1800-01-01 00:00:00;yyyy-MM-dd HH:mm:ss;Asia/Kolkata;SIMPLE;1799-12-31 23:36:32
Jul 9 2023;MMM dd yyyy;Etc/GMT;DATETIME;null
Jul 9 2023;MMM dd yyyy;Etc/GMT;SIMPLE;null
-2023-07-21;DD;Etc/GMT;DATETIME;null
+2023-07-21;DD;Etc/GMT;DATETIME;202
Review Comment:
Why?
##########
ql/pom.xml:
##########
@@ -362,6 +370,12 @@
<groupId>org.apache.orc</groupId>
<artifactId>orc-tools</artifactId>
<version>${orc.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-client-api</artifactId>
Review Comment:
you excluded in the parent pom, I don't think you need to exclude everywhere
now
##########
pom.xml:
##########
@@ -206,7 +207,8 @@
<super-csv.version>2.2.0</super-csv.version>
<tempus-fugit.version>1.1</tempus-fugit.version>
<snappy.version>1.1.10.4</snappy.version>
- <wadl-resourcedoc-doclet.version>1.4</wadl-resourcedoc-doclet.version>
+ <jersey-wadl-doclet.version>4.0.0-M1</jersey-wadl-doclet.version>
+ <jaxb-runtime.version>4.0.0-M1</jaxb-runtime.version>
Review Comment:
use the latest `M2`
##########
ql/src/test/results/clientpositive/llap/groupby3_map_multi_distinct.q.out:
##########
@@ -69,12 +69,12 @@ STAGE PLANS:
minReductionHashAggr: 0.4
mode: hash
outputColumnNames: _col0, _col1, _col2, _col3, _col4,
_col5, _col6, _col7, _col8, _col9, _col10, _col11
- Statistics: Num rows: 500 Data size: 263000 Basic stats:
COMPLETE Column stats: COMPLETE
+ Statistics: Num rows: 307 Data size: 161482 Basic stats:
COMPLETE Column stats: COMPLETE
Review Comment:
how did number of rows reduce?
##########
ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFToUnixTimestamp.java:
##########
@@ -167,7 +167,8 @@ public void testStringArg2() throws HiveException {
runAndVerify(udf2,
new Text("1400-02-01 00:00:00 ICT"),
new Text("yyyy-MM-dd HH:mm:ss z"),
- new LongWritable(TimestampTZUtil.parse("1400-01-31 09:00:22",
ZoneId.systemDefault()).getEpochSecond()));
+ new LongWritable(TimestampTZUtil.parse("1400-01-31 09:24:58",
ZoneId.systemDefault()).getEpochSecond())); // jdk8 & jdk17
use different IANA TZdata versions resulting
Review Comment:
Is this changing the query output? if so, this looks challenging, is there
any way to preserve the old behaviour, some config or so?
##########
serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorUtils.java:
##########
@@ -1654,27 +1652,4 @@ public static boolean
hasAllFieldsSettable(ObjectInspector oi,
private ObjectInspectorUtils() {
// prevent instantiation
}
-
- /**
- * Returns slot value used for ordering the fields to make it deterministic
- * @param field : field of a given class
- * @return
- */
- private static int getSlotValue(Field field) {
Review Comment:
Why are you removing this, you have that ReflectionUtils or so, can't we use
that
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreTimeout.java:
##########
@@ -181,7 +181,7 @@ public void testConnectionTimeout() throws Exception {
try(HiveMetaStoreClient c = new HiveMetaStoreClient(newConf)) {
Assert.fail("should throw connection timeout exception.");
} catch (MetaException e) {
- Assert.assertTrue("unexpected Exception",
e.getMessage().contains("connect timed out"));
+ Assert.assertTrue("unexpected Exception",
e.getMessage().toLowerCase().contains("connect timed out"));
Review Comment:
how did the exception message changed?
##########
standalone-metastore/metastore-server/src/main/sql/derby/hive-schema-4.1.0.derby.sql:
##########
@@ -36,9 +36,9 @@ CREATE TABLE "APP"."DATABASE_PARAMS" ("DB_ID" BIGINT NOT
NULL, "PARAM_KEY" VARCH
CREATE TABLE "APP"."TBL_COL_PRIVS" ("TBL_COLUMN_GRANT_ID" BIGINT NOT NULL,
"COLUMN_NAME" VARCHAR(767), "CREATE_TIME" INTEGER NOT NULL, "GRANT_OPTION"
SMALLINT NOT NULL, "GRANTOR" VARCHAR(128), "GRANTOR_TYPE" VARCHAR(128),
"PRINCIPAL_NAME" VARCHAR(128), "PRINCIPAL_TYPE" VARCHAR(128), "TBL_COL_PRIV"
VARCHAR(128), "TBL_ID" BIGINT, "AUTHORIZER" VARCHAR(128));
-CREATE TABLE "APP"."SERDE_PARAMS" ("SERDE_ID" BIGINT NOT NULL, "PARAM_KEY"
VARCHAR(256) NOT NULL, "PARAM_VALUE" CLOB);
+CREATE TABLE "APP"."SERDE_PARAMS" ("SERDE_ID" BIGINT NOT NULL, "PARAM_KEY"
VARCHAR(256) NOT NULL, "PARAM_VALUE" VARCHAR(32672));
-CREATE TABLE "APP"."COLUMNS_V2" ("CD_ID" BIGINT NOT NULL, "COMMENT"
VARCHAR(4000), "COLUMN_NAME" VARCHAR(767) NOT NULL, "TYPE_NAME" CLOB,
"INTEGER_IDX" INTEGER NOT NULL);
+CREATE TABLE "APP"."COLUMNS_V2" ("CD_ID" BIGINT NOT NULL, "COMMENT"
VARCHAR(4000), "COLUMN_NAME" VARCHAR(767) NOT NULL, "TYPE_NAME" VARCHAR(32672),
"INTEGER_IDX" INTEGER NOT NULL);
Review Comment:
Why are we changing the type, is there any code level change which can
handle these changes. Morover if it is unavoidable, the changes need to be done
in the upgrade files as well
##########
standalone-metastore/metastore-tools/metastore-benchmarks/pom.xml:
##########
@@ -189,18 +189,12 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
- <compilerId>javac-with-errorprone</compilerId>
<forceJavacCompilerUse>true</forceJavacCompilerUse>
</configuration>
<!--
Error Prone integration
-->
<dependencies>
- <dependency>
- <groupId>org.codehaus.plexus</groupId>
- <artifactId>plexus-compiler-javac-errorprone</artifactId>
- <version>${javac.errorprone.version}</version>
- </dependency>
Review Comment:
Why is it removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
