Make parallel restart of already bootstrapped nodes the default - CASS-79 #733
Conversation
burmanm
force-pushed
the
parallel_default
branch
from
3ba431f to
d2996c7
Compare
adejanovski
changed the title
| @@ -672,6 +672,10 @@ func (rc *ReconciliationContext) checkSeedLabels() (int, error) { | |||
| return seedCount, nil | |||
| } | |||
|
|
|||
| func shouldUseFastPath(dc *api.CassandraDatacenter, seedCount int) bool { | |||
| return seedCount > 0 && !(metav1.HasAnnotation(dc.ObjectMeta, api.AllowParallelStartsAnnotations) && dc.Annotations[api.AllowParallelStartsAnnotations] == "false") | |||
There was a problem hiding this comment.
Question: Would it be possible to still have concurrent restarts for nodes which aren't seed nodes?
Issue: Can we modify the changelog to reflect that concurrent starts only applies if there is more than one seed (if not).
There was a problem hiding this comment.
Suggestion: I had to think about this logic for a few minutes. It might be clearer to read if you remove the parenthese, distribute the negation and split it over multiple lines
There was a problem hiding this comment.
This has nothing to do with being a seed node or not, we bootstrap all the nodes. That count is the amount of available seed nodes in the cluster. The seed node is not what user sets or controls. If there are 0 seed nodes, that means there are 0 nodes up or 0 bootstrapped nodes. We will never start multiple nodes that need bootstrapping at the same time so having 0 seed nodes would not really be useful scenario.
The only case this can happen is that the cluster was Stopped and then resumed. In that case, we need to first start one node -> which is then the seed and then start the rest after that one has started.
Removing the parenthese would change the logic and make it at least for me more difficult to read. Now it's two part logic (either we fail in the first one or next ones are true), removing parenthese would make it 3 parts to keep in mind.
Especially since the last two have to be tied together with && or we get a nil pointer (we can't evaluate the value if the annotations are not set).
I'm not entirely sure even what is your suggestion for the alternative logic. Can you code it?
There was a problem hiding this comment.
I'm suggesting:
return seedCount > 0 &&
!metav1.HasAnnotation(dc.ObjectMeta, api.AllowParallelStartsAnnotations) &&
!dc.Annotations[api.AllowParallelStartsAnnotations] == "false"
There was a problem hiding this comment.
That code wouldn't even compile. What you probably meant was:
return seedCount > 0 &&
!metav1.HasAnnotation(dc.ObjectMeta, api.AllowParallelStartsAnnotations) &&
dc.Annotations[api.AllowParallelStartsAnnotations] != "false"
And that changes the logic. Not just because dc.Annotations could be nil in this case (as HasAnnotation checks it's not nil or it would return false, yet now we get false false -> true), but also because the check is different. The test even revealed this.
If dc.Annotations[api.AllowParallelStartsAnnotations] == "true", then we should use the fastpath. The code you proposed returns false in this case, since the annotation has been set and in that case the proposed code would return false. And it's allowed to be set, we should only reject the fastpath if it's set to false, no other case.
burmanm
force-pushed
the
parallel_default
branch
from
d2996c7 to
40e7270
Compare
What this PR does:
The previous fastpath is now made the default behavior. User can disable the fastpath by setting `cassandra.datastax.com/allow-parallel-starts: "false"``
Which issue(s) this PR fixes:
Fixes #731
Checklist