jiajunwang commented on a change in pull request #1413:
URL: https://github.com/apache/helix/pull/1413#discussion_r502596688
##########
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:
"What would happen then?" Then we add other methods to access this
information explicitly. The string return which requires additional parse is a
bad idea in general.
1. What if we change the string format? These methods return string will be
non-backward-compatible. And we don't want to restrict ourselves unnecessarily,
right?
2. Like you add the getOnlineTimestamp now, we can add other methods that
parse the HISTORY string and return the required information.
3. The necessity of testing does not require us to make it public. We can
make the test in the same package. Or read the field directly from ZNRecord.
There are many ways to achieve it and public is the most impactful and costly
(maintenance cost) way. I think it is not good option.
----------------------------------------------------------------
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]
