Skip to content

add missing log #1313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
77yang wants to merge 2 commits into from
Closed

add missing log #1313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4c146f
add missing log
Jul 11, 2019
eabc8bb
add log
Jul 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions broker/src/main/java/org/apache/rocketmq/broker/BrokerStartup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,9 @@ public static BrokerController start(BrokerController controller) {
}

log.info(tip);
System.out.printf("%s%n", tip);
return controller;
} catch (Throwable e) {
e.printStackTrace();
log.error("BrokerStartup fail to boot.", e);
Copy link
Member

@ShannonDing ShannonDing Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, you can print the stack trace to the error log, BUT, it is better to keep the standard output to show the start error directly to the users, in this case, the users can find the error as soon as possible and deal with the error instead of monitor the log file.

Copy link
Author

@77yang 77yang Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShannonDing The users will not know what happened when the program runs in the background if we use e.printStackTrace(); right? And I think it's a more standardized way of writing as an open source middleware. what do you think? 😀

Copy link

@xiangwangcheng xiangwangcheng Jul 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@77yang When the application booted successfully, generally, we redirect the running log to log file. And when it comes to the process of starting the application, I agree with what @ShannonDing said. It is friendly to users if they can know when and how the application failed to start QUICKLY(Of course, log them into file is also necessary).

Copy link
Author

@77yang 77yang Jul 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiangwangcheng So do we need to write both of them at the same time? 😅

Copy link

@xiangwangcheng xiangwangcheng Jul 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I mean.

Copy link
Author

@77yang 77yang Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiangwangcheng I have written both of them now, you can merge this PR.

System.exit(-1);
}

Expand Down Expand Up @@ -143,7 +142,7 @@ public static BrokerController createBrokerController(String[] args) {
MixAll.properties2Object(ServerUtil.commandLine2Properties(commandLine), brokerConfig);

if (null == brokerConfig.getRocketmqHome()) {
System.out.printf("Please set the %s variable in your environment to match the location of the RocketMQ installation", MixAll.ROCKETMQ_HOME_ENV);
log.error("Please set the {} variable in your environment to match the location of the RocketMQ installation", MixAll.ROCKETMQ_HOME_ENV);
System.exit(-2);
}

Expand All @@ -155,9 +154,7 @@ public static BrokerController createBrokerController(String[] args) {
RemotingUtil.string2SocketAddress(addr);
}
} catch (Exception e) {
System.out.printf(
"The Name Server Address[%s] illegal, please set it as follows, \"127.0.0.1:9876;192.168.0.1:9876\"%n",
namesrvAddr);
log.error(String.format("The Name Server Address[%s] illegal, please set it as follows, \"127.0.0.1:9876;192.168.0.1:9876\"%%n", namesrvAddr), e);
System.exit(-3);
}
}
Expand All @@ -169,7 +166,7 @@ public static BrokerController createBrokerController(String[] args) {
break;
case SLAVE:
if (brokerConfig.getBrokerId() <= 0) {
System.out.printf("Slave's brokerId must be > 0");
log.error("Slave's brokerId must be > 0");
System.exit(-3);
}

Expand Down Expand Up @@ -243,6 +240,7 @@ public void run() {
return controller;
} catch (Throwable e) {
e.printStackTrace();
log.error("createBrokerController occur an exception.", e);
System.exit(-1);
}

Expand Down
2 changes: 1 addition & 1 deletion namesrv/src/main/java/org/apache/rocketmq/namesrv/NamesrvController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ private void reloadServerSslContext() {
}
});
} catch (Exception e) {
log.warn("FileWatchService created error, can't load the certificate dynamically");
log.warn("FileWatchService created error, can't load the certificate dynamically", e);
}
}

Expand Down
2 changes: 1 addition & 1 deletion namesrv/src/main/java/org/apache/rocketmq/namesrv/NamesrvStartup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ public static NamesrvController main0(String[] args) {
start(controller);
String tip = "The Name Server boot success. serializeType=" + RemotingCommand.getSerializeTypeConfigInThisServer();
log.info(tip);
System.out.printf("%s%n", tip);
return controller;
} catch (Throwable e) {
e.printStackTrace();
log.error("The Name Server fail to boot.", e);
System.exit(-1);
}

Expand Down
2 changes: 1 addition & 1 deletion namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/ClusterTestRequestProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public RemotingCommand getRouteInfoByTopic(ChannelHandlerContext ctx,
try {
topicRouteData = adminExt.examineTopicRouteInfo(requestHeader.getTopic());
} catch (Exception e) {
log.info("get route info by topic from product environment failed. envName={},", productEnvName);
log.error("get route info by topic from product environment failed. envName=" + productEnvName, e);
}
}

Expand Down
1 change: 1 addition & 0 deletions store/src/main/java/org/apache/rocketmq/store/CommitLog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,7 @@ public DispatchRequest checkMessageAndReturnSize(java.nio.ByteBuffer byteBuffer,
propertiesMap
);
} catch (Exception e) {
log.error("an exception occur in method of checkMessageAndReturnSize", e);
}

return new DispatchRequest(-1, false /* success */);
Expand Down