NealSun96 commented on a change in pull request #1413:
URL: https://github.com/apache/helix/pull/1413#discussion_r502130473
##########
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]
