Optimize isCloudEventsHeader check#445
Conversation
Instead of using: ```java key.substring(0, CE_PREFIX.length()).toLowerCase().startsWith(CE_PREFIX); ``` we can just use: ```java key.regionMatches(true /* ignoreCase */, 0, CE_PREFIX, 0, CE_PREFIX.length()); ``` Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
JemDay
left a comment
JemDay
left a comment
There was a problem hiding this comment.
While you're here .. is there value in adding a generic implementation of isCloudEventsHeader(prefix, key).
Then all of these implementations can be simplified to use that method and remove all the duplicated code
| @Override | ||
| protected boolean isCloudEventsHeader(String key) { | ||
| return key.length() > 3 && key.substring(0, KafkaHeaders.CE_PREFIX.length()).startsWith(KafkaHeaders.CE_PREFIX); | ||
| return key.length() > KafkaHeaders.CE_PREFIX.length() && key.startsWith(KafkaHeaders.CE_PREFIX); |
There was a problem hiding this comment.
Any particular reason this isn't using your new method ?
Also ... Might be worth adding the null-check on key (most of the other readers do this)
There was a problem hiding this comment.
Any particular reason this isn't using your new method ?
it wasn't case-insensitive before like other ones and I think the reason is that other methods are HTTP based (headers normalization) vs this one being Kafka based.
Do you know if the spec is case-insensitive for every defined protocol binding?
Honestly, from a user perspective, I would be surprised if a header isn't considered a CE header due to the case, so I agree with you.
There was a problem hiding this comment.
Hey @JemDay since most of the comments aren't strictly related to the original goal of the PR and the code is strictly equivalent to the existing one, I think, we can follow up on these comments in a different PR/issue.
3 participants
Instead of using:
that allocates 2 new strings, we can just use:
Signed-off-by: Pierangelo Di Pilato pierdipi@redhat.com