Conversation
soenkeliebau
left a comment
soenkeliebau
left a comment
There was a problem hiding this comment.
Left a few questions - might be me misunderstanding the code though
| @Override | ||
| public void onUpdate(Pod oldPod, Pod newPod) { | ||
| cache.putPod(oldPod.getMetadata().getName(), newPod); | ||
| for (PodIP ip : oldPod.getStatus().getPodIPs()) { |
There was a problem hiding this comment.
Should we refer to newPod here instead of oldPod - or both .. ?
There was a problem hiding this comment.
Yes, I think if it is the IPs that have changed, we need to delete the old ones and add the new ones. I'll change that.
| for (String name : names) { | ||
| String resolvedName = resolveDataNodesFromListeners(name, listeners); | ||
| listenerToDataNodeNames.add(resolvedName); | ||
| if (endpoint.getSubsets().isEmpty()) { |
There was a problem hiding this comment.
'endpoint' can be Null here I think?
There was a problem hiding this comment.
Good point - I'll check for that.
There was a problem hiding this comment.
I've added in a few more null checks: f0b5381
| } | ||
| } | ||
| LOG.error("Unable to fetch CRD version for listeners. Returning default value."); | ||
| return "v1alpha1"; |
There was a problem hiding this comment.
What happens downstream if we do this? Do we expose ourselves to worse parse-errors potentially because "someone" think they are handling a v1alpha1, which is not actually the case?
Might it be better to fail "visibly" here?
There was a problem hiding this comment.
This is just an internal check and if the fallback value of v1alpha1 is incorrect, then no listeners will be found and they won't be considered in the endpoint resolution.
| listenerToDataNodeNames.add(resolvedName); | ||
| if (endpoint.getSubsets().isEmpty()) { | ||
| LOG.warn("Endpoint {} has no subsets - pod may be restarting", listenerName); | ||
| return listenerName; |
There was a problem hiding this comment.
This will be mapped to /NotFound down the line - probably correct and intended, just wanted to mention it and see if it sparks a discussion :)
2 participants
Description
Part of #59.
Note
Once merged, we will need to release version 0.4.3 as a separate PR.
Implementation Details
The refactoring is somewhat extensive - which makes the changes difficult to follow - but the changes can be summarised as follows:
TopologyCachebuildNodeToDatanodeMaphas been introduced to cache a map of dataNode names to IPs: this enables O(1) lookup when finding co-located dataNodes for client podsname, try the following in this order (enabling us to short-circuit on the first match):podInformeris used to watch the namespace for pod changes and to thereby reduce the chane of cache-missesHow to test
Definition of Done Checklist
Reviewer
Acceptance