RongtongJin commented on code in PR #10409:
URL: https://github.com/apache/rocketmq/pull/10409#discussion_r3329607484
##########
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:
Could we avoid silently ignoring invalid `when` tokens here, or at least log
a warning when a token cannot be parsed or is outside the 0-23 range? This
helper is used by scheduled maintenance paths such as commitlog cleanup and
topic queue mapping cleanup. If `deleteWhen` or a similar schedule is
misconfigured, returning `false` forever would silently disable the task and
make the operator miss the configuration problem. The previous behavior failed
loudly; changing that to silent ignore seems risky without some visibility.
##########
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:
This test depends on the hour read here matching the hour read inside
`UtilAll.isItTimeToDo`. It can theoretically become flaky if the test crosses
an hour boundary between the two calls. Could we make the positive case
independent of wall-clock timing, for example by passing all valid hours
(`0;1;...;23`) and asserting it returns true, while keeping the invalid-token
cases separate?
--
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]
