[GitHub] [flink] dmvk commented on a diff in pull request #19372: [FLINK-26043][runtime][security] Add periodic kerberos relogin to KerberosDelegationTokenManager

[GitBox <gi...@apache.org>]

dmvk commented on code in PR #19372:
URL: https://github.com/apache/flink/pull/19372#discussion_r872200266


##########
flink-connectors/flink-connector-kinesis/src/test/java/org/apache/flink/streaming/connectors/kinesis/FlinkKinesisConsumerTest.java:
##########
@@ -97,13 +98,14 @@
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.mockStatic;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
 
 /** Suite of FlinkKinesisConsumer tests for the methods called throughout the source life cycle. */
 @RunWith(PowerMockRunner.class)
-@PrepareForTest({FlinkKinesisConsumer.class, KinesisConfigUtil.class})
+@PrepareForTest(FlinkKinesisConsumer.class)
 public class FlinkKinesisConsumerTest extends TestLogger {

Review Comment:
   How does this `Kinesis` change relate to the Kerberos effort?



##########
flink-runtime/src/main/java/org/apache/flink/runtime/security/token/KerberosDelegationTokenManager.java:
##########
@@ -45,10 +56,25 @@ public class KerberosDelegationTokenManager implements DelegationTokenManager {
 
     private final Configuration configuration;
 
+    private final KerberosRenewalPossibleProvider kerberosRenewalPossibleProvider;
+
     @VisibleForTesting final Map<String, DelegationTokenProvider> delegationTokenProviders;
 
-    public KerberosDelegationTokenManager(Configuration configuration) {
+    private final ScheduledExecutor scheduledExecutor;
+
+    private final ExecutorService ioExecutor;

Review Comment:
   `@Nullable`



##########
pom.xml:
##########
@@ -125,7 +125,7 @@ under the License.
 		<junit4.version>4.13.2</junit4.version>
 		<junit5.version>5.8.1</junit5.version>
 		<archunit.version>0.22.0</archunit.version>
-		<mockito.version>2.21.0</mockito.version>
+		<mockito.version>3.4.6</mockito.version>

Review Comment:
   Version upgrades should have the separate issue.
   
   cc @zentol 



##########
pom.xml:
##########
@@ -223,6 +223,14 @@ under the License.
 			<scope>test</scope>
 		</dependency>
 
+		<dependency>
+			<groupId>org.mockito</groupId>
+			<artifactId>mockito-inline</artifactId>
+			<version>${mockito.version}</version>
+			<type>jar</type>
+			<scope>test</scope>
+		</dependency>

Review Comment:
   We try to push against using `Mockito / ...` based mocks as these come with a high maintenance cost. I know there are still some parts of code that use this approach, but it would be great to avoid it for the new code.
   
   Adding new extended capabilities to `Mockito` seems to be heading in the opposite direction.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org