NealSun96 commented on a change in pull request #1413:
URL: https://github.com/apache/helix/pull/1413#discussion_r502129112
##########
File path:
helix-core/src/main/java/org/apache/helix/model/ParticipantHistory.java
##########
@@ -153,16 +185,99 @@ private void updateOfflineHistory(long time) {
if (list.size() == HISTORY_SIZE) {
list.remove(0);
}
-
- DateFormat df = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss:SSS");
- df.setTimeZone(TimeZone.getTimeZone("UTC"));
- String dateTime = df.format(new Date(time));
-
- list.add(dateTime);
+ list.add(historyDateLongToString(time));
}
@Override
public boolean isValid() {
return true;
}
+
+ /*
+ * Parses a history date in string format to its millisecond representation.
+ * Returns -1 if parsing fails.
+ */
+ private static long historyDateStringToLong(String dateString) {
+ SimpleDateFormat simpleDateFormat = new
SimpleDateFormat(HISTORY_DATE_FORMAT);
+ simpleDateFormat.setTimeZone(TimeZone.getTimeZone("UTC"));
+ try {
+ Date date = simpleDateFormat.parse(dateString);
+ return date.getTime();
+ } catch (ParseException e) {
+ LOG.warn("Failed to parse participant history date string: " +
dateString);
+ return -1;
+ }
+ }
+
+ /*
+ * Parses a history date in millisecond to string.
+ */
+ private static String historyDateLongToString(long dateLong) {
+ DateFormat df = new SimpleDateFormat(HISTORY_DATE_FORMAT);
+ df.setTimeZone(TimeZone.getTimeZone("UTC"));
+ return df.format(new Date(dateLong));
+ }
+
+ /**
+ * Parses the session entry map that has been converted to string back to a
map.
+ * NOTE TO CALLER: This assumes the divider between entries is ", " and the
divider between
+ * key/value is "="; if the string is malformed, parsing correctness is not
guaranteed. Always
+ * check if a key is contained before using the key.
+ */
+ public static Map<String, String> parseSessionHistoryStringToMap(String
sessionHistoryString) {
+ sessionHistoryString = sessionHistoryString.substring(1,
sessionHistoryString.length() - 1);
+ Map<String, String> sessionHistoryMap = new HashMap<>();
+
+ for (String sessionHistoryKeyValuePair : sessionHistoryString.split(", "))
{
+ String[] keyValuePair = sessionHistoryKeyValuePair.split("=");
+ if (keyValuePair.length < 2) {
+ LOG.warn("Ignore key value pair while parsing session history due to
missing '=': " +
+ sessionHistoryKeyValuePair);
+ continue;
+ }
+ sessionHistoryMap.put(keyValuePair[0], keyValuePair[1]);
+ }
+
+ return sessionHistoryMap;
+ }
+
+ /*
+ * Take a string session history entry and extract the TIME field out of it.
Return -1 if the TIME
+ * field doesn't exist or if the TIME field cannot be parsed to a long.
+ */
+ private static long extractTimeFromSessionHistoryString(String
sessionHistoryString) {
+ Map<String, String> sessionHistoryMap =
parseSessionHistoryStringToMap(sessionHistoryString);
+ if (!sessionHistoryMap.containsKey(ConfigProperty.TIME.name())) {
+ return -1;
+ }
+ try {
+ return Long.parseLong(sessionHistoryMap.get(ConfigProperty.TIME.name()));
+ } catch (NumberFormatException e) {
+ return -1;
+ }
+ }
+
+ /**
+ * For each entry in History, return its millisecond timestamp; for
timestamps that cannot be
+ * parsed, skip them.
+ */
+ public List<Long> getHistoryTimestampsAsMilliseconds() {
Review comment:
I understand what you mean, and since I also want to call it
getOnlineTimestamps, I made the change.
Though I don't think the old naming is bad: callers get the History
timestamps, as advertised. If they don't know what these are they should read
the class. 😃
##########
File path:
helix-core/src/main/java/org/apache/helix/model/ParticipantHistory.java
##########
@@ -64,6 +69,20 @@ public ParticipantHistory(ZNRecord znRecord) {
super(znRecord);
}
+ /**
+ * @return The list field for HISTORY
+ */
+ public List<String> getHistory() {
+ return _record.getListField(ConfigProperty.HISTORY.name());
+ }
+
+ /**
+ * @return The list field for OFFLINE
+ */
+ public List<String> getOffline() {
+ return _record.getListField(ConfigProperty.OFFLINE.name());
+ }
+
Review comment:
I disagree with this. For a HelixProperty class, if there is a need of a
field, there should be an accessor. It is true that we have timestamp parsers
now, but for example, HISTORY contains other informations too. Down the road,
there may be other code that relies on other HISTORY information. What would
happen then? An accessor will be built. Right now there is a need to these
fields, and I don't see a reason to not create accessors.
For integration test it's not in the same package, so I don't think package
private (nor protected) can help. This is a minor point; there are workarounds.
My main point is the previous paragraph.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
