vamossagar12 commented on code in PR #14371:
URL: https://github.com/apache/kafka/pull/14371#discussion_r1329936115
##########
connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectClusterAssertions.java:
##########
@@ -455,48 +455,42 @@ public void
assertConnectorIsFailedAndTasksHaveFailed(String connectorName, int
}
/**
- * Assert that a connector and its tasks are not running.
+ * Assert that a connector does not exist. This can be used to verify that
a connector has been successfully deleted.
*
* @param connectorName the connector name
* @param detailMessage the assertion message
* @throws InterruptedException
*/
- public void assertConnectorAndTasksAreNotRunning(String connectorName,
String detailMessage)
+ public void assertConnectorDoesNotExist(String connectorName, String
detailMessage)
throws InterruptedException {
try {
waitForCondition(
- () -> checkConnectorAndTasksAreNotRunning(connectorName),
+ () -> checkConnectorDoesNotExist(connectorName),
CONNECTOR_SETUP_DURATION_MS,
- "At least the connector or one of its tasks is still running");
+ "The connector should not exist.");
} catch (AssertionError e) {
throw new AssertionError(detailMessage, e);
}
}
/**
- * Check whether the connector or any of its tasks are still in RUNNING
state
+ * Check whether a connector exists by querying the <strong><em>GET
/connectors/{connector}/status</em></strong> endpoint
*
- * @param connectorName the connector
- * @return true if the connector and all the tasks are not in RUNNING
state; false otherwise
+ * @param connectorName the connector name
+ * @return true if the connector does not exist; false otherwise
*/
- protected boolean checkConnectorAndTasksAreNotRunning(String
connectorName) {
- ConnectorStateInfo info;
+ protected boolean checkConnectorDoesNotExist(String connectorName) {
try {
- info = connect.connectorStatus(connectorName);
+ connect.connectorStatus(connectorName);
} catch (ConnectRestException e) {
return e.statusCode() == Response.Status.NOT_FOUND.getStatusCode();
} catch (Exception e) {
log.error("Could not check connector state info.", e);
return false;
}
- if (info == null) {
- return true;
- }
- return
!info.connector().state().equals(AbstractStatus.State.RUNNING.toString())
- && info.tasks().stream().noneMatch(s ->
s.state().equals(AbstractStatus.State.RUNNING.toString()));
+ return false;
Review Comment:
I don't see true being returned anywhere from this method. Also, I would
assume this condition should be similar to the one being used earlier. i.e
`return info == null ? true : false`
Or am I missing something here?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectClusterAssertions.java:
##########
@@ -455,48 +455,42 @@ public void
assertConnectorIsFailedAndTasksHaveFailed(String connectorName, int
}
/**
- * Assert that a connector and its tasks are not running.
+ * Assert that a connector does not exist. This can be used to verify that
a connector has been successfully deleted.
*
* @param connectorName the connector name
* @param detailMessage the assertion message
* @throws InterruptedException
*/
- public void assertConnectorAndTasksAreNotRunning(String connectorName,
String detailMessage)
+ public void assertConnectorDoesNotExist(String connectorName, String
detailMessage)
throws InterruptedException {
try {
waitForCondition(
- () -> checkConnectorAndTasksAreNotRunning(connectorName),
+ () -> checkConnectorDoesNotExist(connectorName),
CONNECTOR_SETUP_DURATION_MS,
- "At least the connector or one of its tasks is still running");
+ "The connector should not exist.");
Review Comment:
This condition details should be the opposite i.e `The Connector exists` or
something like that, since it's used to construct the message when the assert
failed even after waiting for the timeout. The older message also seems to
suggest the same
##########
connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectClusterAssertions.java:
##########
@@ -455,48 +455,42 @@ public void
assertConnectorIsFailedAndTasksHaveFailed(String connectorName, int
}
/**
- * Assert that a connector and its tasks are not running.
+ * Assert that a connector does not exist. This can be used to verify that
a connector has been successfully deleted.
*
* @param connectorName the connector name
* @param detailMessage the assertion message
* @throws InterruptedException
*/
- public void assertConnectorAndTasksAreNotRunning(String connectorName,
String detailMessage)
+ public void assertConnectorDoesNotExist(String connectorName, String
detailMessage)
Review Comment:
Given that we know this method is only being referenced from connector
deletion cases, why don't we just rename it to `assertConnectorDeleted`. As
such, the connector won't exist only when it's deleted? For ] cases when we
want to verify if the connector has been created successfully, we will have
asserts for checking that and don't need to rely on this method anyways. WDYT?
--
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]
