RongtongJin commented on code in PR #10409:
URL: https://github.com/apache/rocketmq/pull/10409#discussion_r3434174146
##########
common/src/main/java/org/apache/rocketmq/common/UtilAll.java:
##########
@@ -117,13 +117,29 @@ public static long computeElapsedTimeMilliseconds(final
long beginTime) {
}
public static boolean isItTimeToDo(final String when) {
+ if (StringUtils.isBlank(when)) {
+ return false;
+ }
+
String[] whiles = when.split(";");
if (whiles.length > 0) {
Calendar now = Calendar.getInstance();
+ int nowHour = now.get(Calendar.HOUR_OF_DAY);
for (String w : whiles) {
- int nowHour = Integer.parseInt(w);
- if (nowHour == now.get(Calendar.HOUR_OF_DAY)) {
- return true;
+ if (StringUtils.isBlank(w)) {
+ continue;
+ }
+ String trimmed = w.trim();
+ try {
+ int hour = Integer.parseInt(trimmed);
+ if (hour < 0 || hour > 23) {
+ continue;
+ }
+ if (hour == nowHour) {
+ return true;
+ }
+ } catch (NumberFormatException ignored) {
Review Comment:
I still consider this blocking. Silently accepting an invalid schedule can
hide a bad `deleteWhen`-style configuration and leave scheduled maintenance
disabled without any clear signal. Please keep configuration mistakes
observable, either by preserving fail-fast behavior or by logging/validating
invalid tokens explicitly.
##########
common/src/test/java/org/apache/rocketmq/common/UtilAllTest.java:
##########
@@ -151,6 +153,20 @@ public void testSplit() {
assertEquals(Collections.EMPTY_LIST, UtilAll.split("", comma));
}
+ @Test
+ public void testIsItTimeToDo() {
+ int currentHour = Calendar.getInstance().get(Calendar.HOUR_OF_DAY);
+ assertTrue(UtilAll.isItTimeToDo(String.valueOf(currentHour)));
+ assertTrue(UtilAll.isItTimeToDo("foo; " + currentHour + " ; 25"));
+ assertTrue(UtilAll.isItTimeToDo(" " + currentHour + " "));
+
+ assertFalse(UtilAll.isItTimeToDo(null));
Review Comment:
Still worth fixing before merge. This can be made deterministic by passing
all valid hours (`0;1;...;23`) for the positive case, and by adding a
valid-but-non-current hour negative case.
--
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]
