kezhenxu94 commented on code in PR #13399:
URL:
https://github.com/apache/dolphinscheduler/pull/13399#discussion_r1070575057
##########
dolphinscheduler-service/src/test/java/org/apache/dolphinscheduler/service/log/LogClientTest.java:
##########
@@ -151,8 +151,7 @@ public void testRemoveTaskLog() throws Exception {
.thenReturn(command);
LogClient logClient = new LogClient();
- Boolean status =
logClient.removeTaskLog(Host.of("localhost:1234"), "/log/path");
- Assertions.assertTrue(status);
+ Assertions.assertDoesNotThrow(() ->
logClient.removeTaskLog(Host.of("localhost:1234"), "/log/path"));
Review Comment:
Because you have a `catch (Exception e)` in `logClient.removeTaskLog`
method, the `removeTaskLog` won't throw exception anyhow, so there is no point
to assert it does not throw.
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/log/LogClient.java:
##########
@@ -179,27 +178,14 @@ public byte[] getLogBytes(String host, int port, String
path) {
* @param path path
* @return remove task status
*/
- public Boolean removeTaskLog(@NonNull Host host, String path) {
Review Comment:
Let's also remove the `@return` statement in javadoc
##########
dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/log/LogClient.java:
##########
@@ -179,27 +178,14 @@ public byte[] getLogBytes(String host, int port, String
path) {
* @param path path
* @return remove task status
*/
- public Boolean removeTaskLog(@NonNull Host host, String path) {
+ public void removeTaskLog(@NonNull Host host, String path) {
logger.info("Remove task log from host: {} logPath {}", host, path);
RemoveTaskLogRequestCommand request = new
RemoveTaskLogRequestCommand(path);
try {
Command command = request.convert2Command();
- Command response = this.client.sendSync(host, command,
LOG_REQUEST_TIMEOUT);
- if (response != null) {
- RemoveTaskLogResponseCommand taskLogResponse =
- JSONUtils.parseObject(response.getBody(),
RemoveTaskLogResponseCommand.class);
- return taskLogResponse.getStatus();
- }
- return false;
- } catch (InterruptedException ex) {
- Thread.currentThread().interrupt();
- logger.error(
- "Remove task log from host: {}, logPath: {} error, the
current thread has been interrupted",
- host, path, ex);
- return false;
+ client.send(host, command);
Review Comment:
Because we change it from sync to async, can we add a callback and log the
error if there is any? Otherwise it's hard to debug if it failed to remove the
task log.
--
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]
