Aggregations Refactor: Refactor Range Aggregations#15399
Aggregations Refactor: Refactor Range Aggregations#15399colings86 merged 1 commit intoelastic:feature/aggs-refactoringfrom colings86:refactor/rangeAgg
Conversation
There was a problem hiding this comment.
I don't have a good alternative to propose but wish we did not have to add this public method to Cidrs solely for testing purposes
There was a problem hiding this comment.
if we keep it here, maybe we should at least require an address that is valid for the given mask like cidrMaskToMinMax expects?
There was a problem hiding this comment.
So the reason for this method was so I could take a random IP address and a random networkMask and turn them into an appropriate CIDR. I'm not sure how to do that without either having this method or recreating a lot of the logic in this class in the test?
There was a problem hiding this comment.
agreed that's what I said I don't have a good alternative so let's keep it. But can we make it less lenient and expect an ip address that is compatible with the mask and move the - (ipAddress & (blockSize - 1)) part to the ipv4 range tests?
|
I left two minor comments but it looks good to me otherwise. |
No description provided.