npawar commented on a change in pull request #6667:
URL: https://github.com/apache/incubator-pinot/pull/6667#discussion_r595524502
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/protocols/SegmentCompletionProtocol.java
##########
@@ -180,6 +184,16 @@ private Request(Params params, String msgType) {
}
public String getUrl(String hostPort, String protocol) {
+ String streamPartitionMsgOffset;
+ try {
+ streamPartitionMsgOffset = _params.getStreamPartitionMsgOffset() ==
null ? null :
+ URLEncoder.encode(_params.getStreamPartitionMsgOffset(),
StandardCharsets.UTF_8.toString());
Review comment:
That would mean making the `toString` method of the
`StreamPartitionOffset` impl return a url encoded string, which is incorrect.
The impl shouldn't always url encode the toString representation. I see 2
options:
1. Adding a getURLEncodedString method in the impl
2. Move this logic inside `withStreamPartitionMsgOffset`. I think it is fair
to make this method assume that stream partition offsets can be complex string
representations and that it should encode them before accepting them into
params.
Lmk which option you prefer.
----------------------------------------------------------------
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]
