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

Deprecate ClusterState types #2434

Closed
wants to merge 1 commit into from
Closed

Conversation

muhamadazmy
Copy link
Contributor

@muhamadazmy muhamadazmy commented Dec 17, 2024

Deprecate ClusterState types

Summary:
This commit moves cluster.proto to deprecated_cluster.proto mainly but it also does
the following:

  • Extract cluster configuration types to their own file.
  • Extract partition processor types (Status, RunMode, etc...) to their own file.

Stack created with Sapling. Best reviewed with ReviewStack.

This was referenced Dec 17, 2024
@muhamadazmy muhamadazmy force-pushed the pr2434 branch 2 times, most recently from de80d30 to d896c99 Compare December 17, 2024 14:03
@muhamadazmy muhamadazmy marked this pull request as draft December 17, 2024 15:09
@muhamadazmy muhamadazmy force-pushed the pr2434 branch 3 times, most recently from 729d7dd to 16764bb Compare December 17, 2024 17:49
@muhamadazmy muhamadazmy changed the title cluster state integration with cluster controller Cluster state integration with cluster controller Dec 17, 2024
@muhamadazmy muhamadazmy force-pushed the pr2434 branch 4 times, most recently from f8d3c6c to 1e57e8a Compare December 20, 2024 13:13
@muhamadazmy muhamadazmy changed the title Cluster state integration with cluster controller Deprecate ClusterState types Dec 20, 2024
@muhamadazmy muhamadazmy force-pushed the pr2434 branch 2 times, most recently from 164b95a to 9a553d2 Compare December 20, 2024 14:58
Copy link
Contributor

@AhmedSoliman AhmedSoliman left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 13 to 19
import "restate/common.proto";
import "restate/cluster.proto";
import "google/protobuf/empty.proto";
import "google/protobuf/duration.proto";
import "google/protobuf/timestamp.proto";

package restate.deprecated_cluster;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that this file is a simple rename that github doesn't recognize as a rename, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's confused because we still have a file with the same old name (cluster.proto)

@muhamadazmy muhamadazmy marked this pull request as ready for review December 26, 2024 08:51
@muhamadazmy muhamadazmy marked this pull request as draft December 26, 2024 11:44
@muhamadazmy muhamadazmy force-pushed the pr2434 branch 2 times, most recently from 9ae265e to fe0a9fe Compare December 26, 2024 14:01
This was referenced Dec 27, 2024
@muhamadazmy muhamadazmy marked this pull request as ready for review January 6, 2025 09:28
Summary:
This commit moves cluster.proto to deprecated_cluster.proto mainly but it also does
the following:
- Extract cluster configuration types to their own file.
- Extract partition processor types (Status, RunMode, etc...) to their own file.
@AhmedSoliman
Copy link
Contributor

Is this ready for review? I see that it needs rebasing.

@muhamadazmy
Copy link
Contributor Author

The changes Till made regarding "Provisioning" are closely intertwined with mine, making it impossible to resolve with a simple rebase. I'll need to start over from scratch, but that's okay—it’s mostly mechanical work.

@tillrohrmann
Copy link
Contributor

Sorry for causing you trouble with my changes. I wasn't aware of the dependency.

@muhamadazmy
Copy link
Contributor Author

@tillrohrmann it's totally fine. No worries :D at all.

I will pick this up again after the meeting based on the priorities.

@muhamadazmy muhamadazmy closed this Jan 8, 2025
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.

3 participants