NIFI-9281 Enable Building on Java 17 by exceptionfactory · Pull Request #5870 · apache/nifi

exceptionfactory · 2022-03-16T12:25:06Z

Description of PR

NIFI-9281 Updates a number of tests and several dependencies to support building the project on Java 17. The majority of the changes involve adjustments to unit tests in order to avoid illegal reflective access, which throws an error by default on Java 17.

Dependency updates include upgrading the Maven WAR Plugin, the QuestDB driver for NiFi Framework status history, and the version of Groovy used in several Graph Processors. The GitHub actions workflow includes a new build target for Java 17 on Ubuntu Linux.

Although these changes enable a build across the project, some extension components may require further changes for full runtime support. Additional changes for particular components can be addressed in subsequent pull requests.

The following list summarizes the changes:

Added Ubuntu Zulu JDK 17 GitHub build
Adjusted MiNiFi C2 FileSystemConfigurationCache test to avoid using environment variables
Adjusted MiNiFi StatusLogger and StatusLoggerTest to avoid overriding private logger
Adjusted failure reason attribute check in TestGetIgniteCache
Adjusted TestRangerAuthorizer and TestRangerNiFiAuthorizer to avoid checking nested exceptions
Adjusted encrypt-config TestUtil to avoid unnecessary comparison of different types
Disabled Javascript tests on Java 15 and higher due to removal of Nashorn from the JRE
Disabled several Hive 3 tests on Java 17 for StringInternUtils illegal access
Refactored nifi-enrich-processors tests to use Mockito without Powermock
Refactored nifi-toolkit-tls tests to avoid illegal reflective access
Removed deprecated X509Certificate test in CertificateUtilsTest
Removed kryo serialization from nifi-site-to-site-client test
Updated TestHashContent to use SHA-1 instead of SHA for hash algorithm
Upgraded maven-war-plugin from 2.5 to 3.3.2
Upgraded nifi-graph-bundle dependencies from Groovy 2.5.14 to 3.0.8
Upgraded QuestDB from 4.2.1 to 6.2.1 in nifi-framework-core

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
Have you verified that the full build is successful on JDK 8?
Have you verified that the full build is successful on JDK 11?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

- Added Ubuntu Zulu JDK 17 GitHub build - Adjusted MiNiFi C2 FileSystemConfigurationCache test to avoid using environment variables - Adjusted MiNiFi StatusLogger and StatusLoggerTest to avoid overriding private logger - Adjusted failure reason attribute check in TestGetIgniteCache - Adjusted TestRangerAuthorizer and TestRangerNiFiAuthorizer to avoid checking nested exceptions - Adjusted encrypt-config TestUtil to avoid unnecessary comparison of different types - Disabled Javascript tests on Java 15 and higher - Disabled several Hive 3 tests on Java 17 for StringInternUtils illegal access - Refactored nifi-enrich-processors tests to use Mockito without Powermock - Refactored nifi-toolkit-tls tests to avoid illegal reflective access - Removed deprecated X509Certificate test in CertificateUtilsTest - Removed kryo serialization from nifi-site-to-site-client test - Updated TestHashContent to use SHA-1 instead of SHA for hash algorithm - Upgraded maven-war-plugin from 2.5 to 3.3.2 - Upgraded nifi-graph-bundle dependencies from Groovy 2.5.14 to 3.0.8 - Upgraded QuestDB from 4.2.1 to 6.2.1 in nifi-framework-core

MikeThomsen · 2022-03-16T21:24:44Z

I'll try to review this later tonight or tomorrow morning. Would love to have an excuse to go to Java 17 on the job :-D

greyp9

This all looks really solid. I tackled this by building from HEAD using a fresh Java 17 install, and using the PR as a reference. Had a couple of questions about fidelity of test cases, but nothing dire.

greyp9 · 2022-03-16T18:26:18Z

...-to-site-client/src/test/java/org/apache/nifi/remote/client/socket/TestSiteToSiteClient.java

-        }
-    }
-
+    @SuppressWarnings("deprecation")


There seems to be a good deal of alternate test coverage for the three nifi classes under test here. But another option would be to replace the problematic com.esotericsoftware usage. (just making notes...)

The kryo dependency did not have any direct runtime references, as indicated by the test scope, so other than rewriting the tests using standard Java Obejct serialization, I'm not sure what would be considered a comparable approach.

greyp9 · 2022-03-16T20:03:32Z

...-bundle/nifi-enrich-processors/src/test/java/org/apache/nifi/processors/TestISPEnrichIP.java

        final Map<String, String> attributes = new HashMap<>();
        attributes.put("ip", "somenonexistentdomain.comm");

-        when(InetAddress.getByName("somenonexistentdomain.comm")).thenThrow(UnknownHostException.class);


Why don't we need the Mockito.when anymore?

The purpose of this mock was to ensure that this lookup would always throw an UnknownHostException. Unfortunately, static mocking can be problematic in some cases. Removing the mock resulted in the expected exception being thrown.

greyp9 · 2022-03-16T20:31:37Z

...undle/nifi-hive3-processors/src/test/java/org/apache/hive/streaming/TestNiFiRecordSerDe.java

 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;

+@DisabledOnJre(value = JRE.JAVA_17, disabledReason = "Hive3 StringInternUtils illegal reflective access")


Definitely a benefit of moving to junit 5!

greyp9 · 2022-03-16T20:44:04Z

...ite-processors/src/test/java/org/apache/nifi/processors/ignite/cache/TestGetIgniteCache.java

        assertEquals(1, getFailureFlowFiles.size());

-        final MockFlowFile getOut = getRunner.getFlowFilesForRelationship(GetIgniteCache.REL_FAILURE).get(0);
-        getOut.assertAttributeEquals(GetIgniteCache.IGNITE_GET_FAILED_REASON_ATTRIBUTE_KEY,


Assertions.assertTrue(getOut.getAttribute(GetIgniteCache.IGNITE_GET_FAILED_REASON_ATTRIBUTE_KEY).contains( GetIgniteCache.IGNITE_GET_FAILED_MESSAGE_PREFIX + "java.lang.NullPointerException"));

Looks like the new exception is purely additive, so we could constrain the value safely.

greyp9 · 2022-03-16T20:55:55Z

...nger-plugin/src/test/java/org/apache/nifi/ranger/authorization/TestRangerNiFiAuthorizer.java

-            cause = cause.getCause();
-        }
-        assertTrue(foundLoginException);
+        assertThrows(AuthorizerCreationException.class, () -> authorizer.onConfigured(configurationContext));


Not sure how important it is, but we're losing some precision here.

Yes, this does make the test looser in terms of expected exception checking. There appear to be other issues with Ranger and Java 17, so the other option is marking several of these methods as ignored on Java 17, but it seemed better to make the tests somewhat more general in this scenario.

greyp9 · 2022-03-16T21:19:03Z

nifi-toolkit/nifi-toolkit-tls/src/test/java/org/apache/nifi/toolkit/tls/util/TlsHelperTest.java


    private File file;

-    private static void setUnlimitedCrypto(boolean value) {


Does this removal imply that the unlimited strength crypto is expected, or are we saying that it shouldn't matter?

Good question, this change implies that unlimited strength cryptography support is expected. Java 8 Update 171 introduced this by default, so there should be a separate effort to remove all checks for unlimited strength cryptography.

Yep, you're right! I remember both policies being included in the distribution, but was fuzzy on the details.

https://bugs.openjdk.java.net/browse/JDK-8170157

greyp9 · 2022-03-16T21:20:52Z

...oolkit/nifi-toolkit-tls/src/test/java/org/apache/nifi/toolkit/tls/util/PasswordUtilTest.java

-        SecureRandom secureRandom = mock(SecureRandom.class);
+        SecureRandom secureRandom = new SecureRandom();
        PasswordUtil passwordUtil = new PasswordUtil(secureRandom);
-        int value = 8675309;


greyp9 · 2022-03-16T21:48:34Z

...gistry-ranger-plugin/src/test/java/org/apache/nifi/registry/ranger/TestRangerAuthorizer.java

-            cause = cause.getCause();
-        }
-        assertFalse(foundOtherException);
+        assertThrows(SecurityProviderCreationException.class, () -> setup(registryProperties, mock(UserGroupProvider.class), configurationContext));


Are we losing anything significant here?

As mentioned in relation to the other Ranger test changes, this does make the exception checking looser, trading off some precision for general test execution on all platforms.

greyp9

Started up a dev instance (compile=17, runtime=17), and tested with a simple QueryDatabaseTableRecord -> PutDatabaseRecord flow; worked perfectly.

I imagine there will be runtime rough spots around the edges, but this is a solid start. LGTM.

MikeThomsen · 2022-03-17T18:06:39Z

I imagine there will be runtime rough spots around the edges, but this is a solid start. LGTM.

Same here. Going to merge because nothing jumped out at me as problematic with the changes, and it appears to run the whole test suite just fine w/ Java 17. Anything we find problematic going forward that's broken will probably be found through live trial and error.

- Added Ubuntu Zulu JDK 17 GitHub build - Adjusted MiNiFi C2 FileSystemConfigurationCache test to avoid using environment variables - Adjusted MiNiFi StatusLogger and StatusLoggerTest to avoid overriding private logger - Adjusted failure reason attribute check in TestGetIgniteCache - Adjusted TestRangerAuthorizer and TestRangerNiFiAuthorizer to avoid checking nested exceptions - Adjusted encrypt-config TestUtil to avoid unnecessary comparison of different types - Disabled Javascript tests on Java 15 and higher - Disabled several Hive 3 tests on Java 17 for StringInternUtils illegal access - Refactored nifi-enrich-processors tests to use Mockito without Powermock - Refactored nifi-toolkit-tls tests to avoid illegal reflective access - Removed deprecated X509Certificate test in CertificateUtilsTest - Removed kryo serialization from nifi-site-to-site-client test - Updated TestHashContent to use SHA-1 instead of SHA for hash algorithm - Upgraded maven-war-plugin from 2.5 to 3.3.2 - Upgraded nifi-graph-bundle dependencies from Groovy 2.5.14 to 3.0.8 - Upgraded QuestDB from 4.2.1 to 6.2.1 in nifi-framework-core This closes apache#5870 Signed-off-by: Mike Thomsen <mthomsen@apache.org>

greyp9 reviewed Mar 16, 2022

View reviewed changes

greyp9 approved these changes Mar 17, 2022

View reviewed changes

MikeThomsen approved these changes Mar 17, 2022

View reviewed changes

asfgit closed this in 412e21b Mar 17, 2022


		private File file;

		private static void setUnlimitedCrypto(boolean value) {

Conversation

exceptionfactory commented Mar 16, 2022

Description of PR

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

MikeThomsen commented Mar 16, 2022

Uh oh!

greyp9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greyp9 left a comment

Choose a reason for hiding this comment

Uh oh!

MikeThomsen commented Mar 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants