gharris1727 commented on code in PR #12789:
URL: https://github.com/apache/kafka/pull/12789#discussion_r1008315241
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java:
##########
@@ -192,6 +193,41 @@ public void testConnectorStatus() {
ConnectorStateInfo state = herder.connectorStatus(connectorName);
assertEquals(connectorName, state.name());
+ assertEquals(ConnectorType.UNKNOWN, state.type());
+ assertEquals("RUNNING", state.connector().state());
+ assertEquals(1, state.tasks().size());
+ assertEquals(workerId, state.connector().workerId());
+
+ ConnectorStateInfo.TaskState taskState = state.tasks().get(0);
+ assertEquals(0, taskState.id());
+ assertEquals("UNASSIGNED", taskState.state());
+ assertEquals(workerId, taskState.workerId());
+ }
+
+ @Test
+ public void testConnectorStatusMissingPlugin() {
+ ConnectorTaskId taskId = new ConnectorTaskId(connectorName, 0);
+
+ AbstractHerder herder = mock(AbstractHerder.class, withSettings()
+ .useConstructor(worker, workerId, kafkaClusterId, statusStore,
configStore, noneConnectorClientConfigOverridePolicy)
+ .defaultAnswer(CALLS_REAL_METHODS));
+
+ when(plugins.newConnector(anyString())).thenThrow(new
ConnectException("Unable to find class"));
Review Comment:
Confirmed that this error reaches the connectorStatus call without the patch
applied.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java:
##########
@@ -700,6 +707,9 @@ public ConnectorType connectorTypeForClass(String
connClass) {
* @return the {@link ConnectorType} of the connector
*/
public ConnectorType connectorTypeForConfig(Map<String, String>
connConfig) {
Review Comment:
After doing some archaeology on the two null checks that are replaced by
this one:
* https://github.com/apache/kafka/pull/10822
* https://github.com/apache/kafka/pull/3812
It appears that the null check is required when the method uses information
from both the status backing store and the config backing store. In some
situations, the config can be null while there are statuses for the connector
and/or tasks:
* After a connector is created, a worker reads the new statuses without
reading the new configs.
* After a connector is deleted, a worker reads the (absence of) new configs
without reading the cleared statuses.
I think these are relatively narrow windows of opportunity for an NPE, but
they still exist. In the same spirit as the current change, it would be better
to return UNKNOWN in these situations than throw an NPE.
I'll update the method name and Javadoc.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
