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-6493: Initial implementation of Debezium Operator #1

Merged
merged 27 commits into from
Jun 8, 2023

Conversation

jcechace
Copy link
Member

No description provided.

@jcechace
Copy link
Member Author

@jpechane So the changes discussed on the call should be in

  1. externalConfiguration was renamed to runtime
  2. restructured sink section to include sink.type and sink.config
  3. restructured source section to include source.class and source.config

For the default/automatic naming of predicates. I'm still not sure about it. You are right that the name is optional, however the predicate has to be referenced in transformation and I think explicit is better than implicit (guessing / researching the implicit naming) here.

The default naming could be applied to transformations instead of predicates, however then the two will be different as one will be named implicitly and the other explicitly.

@jpechane
Copy link
Contributor

jpechane commented Jun 1, 2023

@jcechace In that case predicates should not be list. It should be like

    pred2:

etc.

WDYT?

@jcechace
Copy link
Member Author

jcechace commented Jun 1, 2023

@jcechace In that case predicates should not be list. It should be like

    pred2:

etc.

WDYT?

You mean?

predicates: 
  pred0:
    type: String
    # other preticate property
  pred1:
    type: String
    # other preticate property

Sure, I don't have a strong prefference in this case (it probably does look a bit better (though it's a bit worse java wise as the "name" is pulled out into a map key instead of being part of the Predicate POJO).

While we are at it. Wold you object if we also change a format a bit (to corespond to how we handle dynamic properties in sink and source)?

So it would be

predicates: 
  pred0:
    type: String
    config:
      # other predicate property
  pred1:
    type: String
    config:
      # other predicate property

The only thing I don't like is that here type is a class, while sink.type is a name (so is going to be source.type in the future). Nothing can be done about this though...

@jpechane
Copy link
Contributor

jpechane commented Jun 1, 2023

Maybe we can have both class and type and have a config file that will map most commonly used predicate classes into logical names?

@jcechace
Copy link
Member Author

jcechace commented Jun 1, 2023

sure. Lets call it class for now and introduce the type later. Provided you don't mind that class will become type in the generated application.properties (so there won't be 1:1 matching between property configuration and the CR representation). Personally I think this is bound to happen sooner or later if we want to have a convenient configuration

@jcechace
Copy link
Member Author

jcechace commented Jun 1, 2023

@jpechane BTW any objections against generalising the pattern where the dynamic properteis are placed under the config attribute? So this would be the case also for e.g format

format:
  value:
    type: String
    config:
      # other format properties

or Transformers (which would also be changed to map from list)

transforms:
  trans0:   
    type: String
    predicate: String
    negate: Boolean
    config:
      # other transformation properties

I'd like to be consistent with these

@jcechace
Copy link
Member Author

jcechace commented Jun 6, 2023

@jpechane For now there is only a single PR workflow which doesn't pull any other repo for now. Building the server itself wouldn't help much -- we also need to build the DS container image and run the tests against a Kind cluster with this image loaded.

I will eventually add all of it together with better testing, however it has to wait until after devconf.

@jpechane jpechane merged commit cee675f into debezium:main Jun 8, 2023
@jpechane
Copy link
Contributor

jpechane commented Jun 8, 2023

@jcechace Applied, thanks!

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