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

DBZ-8512 Support storages supported by Debezium operator for pipeline in Debezium platform #8

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

mfvitale
Copy link
Member

@mfvitale mfvitale commented Jan 2, 2025

@mfvitale mfvitale marked this pull request as draft January 2, 2025 16:13
@mfvitale
Copy link
Member Author

mfvitale commented Jan 2, 2025

Before merging the fix debezium/debezium-operator#81 must be released and the new operator helm chart version must be used.

@mfvitale mfvitale requested review from jcechace and jpechane January 2, 2025 16:15
@jcechace
Copy link
Member

jcechace commented Jan 2, 2025

We should have a call about this.. I have a feeling that once again this is trying to replicate and allow absolutely all possible configurations. The main idea behind the UI is to provided opinionated means.

Just looking at all the hard coded class and property names looks like an issue in making.

@mfvitale
Copy link
Member Author

mfvitale commented Jan 3, 2025

I have a feeling that once again this is trying to replicate and allow absolutely all possible configurations.

I think that we talked about to use JDBC as default storage (per deployment) not the only supported one. Isn't it?

We also have another feature in the backlog to let user choose at runtime the preferred storage.

So with this PR the backend supports all the possible storage supported by the operator and on the other hand the helm chart offers an easy way to provide settings for the JDBC storage. More advanced user can still use another different storage by overriding conductor properties via envs.

Just looking at all the hard coded class and property names looks like an issue in making.

This depends on the operator model. Honestly I would have avoided the use of the storage naming and just used the class and the list of generic config just as a pass-trough. This would have simplified the things also in that case. Am I loosing something?

@jcechace
Copy link
Member

jcechace commented Jan 3, 2025

I have a feeling that once again this is trying to replicate and allow absolutely all possible configurations.

I think that we talked about to use JDBC as default storage (per deployment) not the only supported one. Isn't it?

We also have another feature in the backlog to let user choose at runtime the preferred storage.

So with this PR the backend supports all the possible storage supported by the operator and on the other hand the helm chart offers an easy way to provide settings for the JDBC storage. More advanced user can still use another different storage by overriding conductor properties via envs.

Sure, but the way to model (and implement it) should be different. It ties back to that "component registry" / "config schema". Storage should be added as a configurable entity -- similar how Transformations are intended to be in the end. Essentially there shouldn't be a hardcoded dependency on particular implementation. |

It's fine to hardcode it for the JDBC for now but doing so for all of them just seems too much.

@mfvitale
Copy link
Member Author

mfvitale commented Jan 3, 2025

Storage should be added as a configurable entity -- similar how Transformations are intended to be in the end.

This sounds good but then also the DS resource should be able to manage this flexibility. As of now this means to use the custom store from the DS resource in that way the type of storage and the configs can be passed without any logic. Was you thinking to this approach?

This can be a solution but then why to not have the same simplification on the DS operator?

@jcechace
Copy link
Member

jcechace commented Jan 6, 2025

Storage should be added as a configurable entity -- similar how Transformations are intended to be in the end.

This sounds good but then also the DS resource should be able to manage this flexibility. As of now this means to use the custom store from the DS resource in that way the type of storage and the configs can be passed without any logic. Was you thinking to this approach?

This can be a solution but then why to not have the same simplification on the DS operator?

I think we should have a call about this since e.g. I don't really follow the note about DS. I will ping you in second half of this week.

@mfvitale
Copy link
Member Author

@jcechace can you give another look?

@jcechace
Copy link
Member

@mfvitale sorry for the delay, busy few days. Mostly LGTM except for the one thing I commented on.

@mfvitale mfvitale marked this pull request as ready for review January 16, 2025 15:15
@mfvitale mfvitale merged commit c9930cc into debezium:main Jan 17, 2025
2 checks passed
@mfvitale mfvitale deleted the DBZ-8512 branch January 17, 2025 16:47
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