Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ObservedGeneration to solrcloud status #722

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RavinaChidambaram
Copy link

Implemented #650

Have added observed generation to the status of the solrcloud CR to track the transition of the solrcloud.

Copy link
Contributor

@gerlowskija gerlowskija left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one in-line question, but otherwise this LGTM. It seems to bring out status a little closer into line with best practices.

I'm ambivalent on the "lastUpdated" timestamp field that was described in the issue description. It seems like there's less strong of a precedent around that field, and it's maybe more subject to some of the "false-signal" concerns I expressed in #650. But I'm not against it - if others think it's worthwhile and are willing to add it, I'll hold off merging for a bit. But otherwise I'll add a Chart.yaml change-entry and merge this sometime in the next few days!

Thanks @RavinaChidambaram !

@@ -1160,6 +1160,9 @@ type SolrCloudStatus struct {
// BackupRepositoriesAvailable lists the backupRepositories specified in the SolrCloud and whether they are available across all Pods.
// +optional
BackupRepositoriesAvailable map[string]bool `json:"backupRepositoriesAvailable,omitempty"`

// ObservedGeneration represents the most recent generation observed for this SolrCloud.
ObservedGeneration int64 `json:"observedGeneration"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Are there any edge-cases where there won't be an observedGeneration, say, immediately after creating a SolrCloud but before the operator has created any of the resources backing it?

Assuming that case exists, I guess the 'observedGeneration' would just be '0' in that case, and that's probably fine unless it'd be better to make the field an optional *int64?

@RavinaChidambaram
Copy link
Author

Thanks for the feedback and approval. I’m currently working on considering other scenarios as well around observedGeneration, and I’ll address that within this PR. Please hold off on merging for a bit while I make these adjustments.

Also lets just address observedGeneration in this PR and then add lastTransitionTime in another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants