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

Add normalization to destination definition and actor definition table #18300

Merged
merged 17 commits into from
Nov 4, 2022

Conversation

andriikorotkov
Copy link
Contributor

@andriikorotkov andriikorotkov commented Oct 21, 2022

What

updated StandardDestinationDefinition.yaml, added normalization and tags to the destination_definition.yaml and added information about normalization and DBT to the ACTOR_DEFINITION table

How

I added the fields we need for DBT and Normalization in StandardDestinationDefinition.yaml. Also, I expanded the ACTOR_DEFINITION table a bit, in order to store the values of destinations normalization and DBT where needed (the new fields can be null, since the ACTOR_DEFINITION table also stores information about sources and destinations that do not support normalization). Since the path by which the migration file was created has already been added to gradle, during the build of the platform, the new migration will be performed, and the fields will be added to the table.

Recommended reading order

  1. StandardDestinationDefinition.yaml
  2. V0_40_15_001__AddActorDefinitionNormalizationAndDbtColumns.java
  3. ConfigWriter.java

🚨 User Impact 🚨

Currently, it does not add any impact to the platform. But in the future, we will use values for normalization and DBT fields from the definition file.

…ags to the destination_definition.yaml and added information about normalization and DBT to the ACTOR_DEFINITION table
@andriikorotkov andriikorotkov temporarily deployed to more-secrets October 21, 2022 14:31 Inactive
@andriikorotkov andriikorotkov linked an issue Oct 21, 2022 that may be closed by this pull request
@andriikorotkov
Copy link
Contributor Author

@evantahler @tuliren @pedroslopez, I plan to completely close the first step from the document "Decouple Normalization from Platform" with this pull request. In my opinion, this is all that should be done in the first step, but I could be wrong and miss something. If I've done something wrong, please let me know and I'll fix it.

@alafanechere
Copy link
Contributor

Hey @andriikorotkov could you please create an issue related to this PR?
If I understand correctly you try to achieve these steps from the tech-spec right?

Step 1: update StandardDestinationDefinition.yaml
Add the new fields to the definition
Regenerate the model class locally
Copy the information from the spec file to the definition file

As I'm not very familiar with the ConfigWriter and the migrations would you mind adding more details to the How section? I'd also appreciate the User Impact to be filled to share how this will be deployed and could be rolled back.
Feel free to change the status to Ready for review when it's ready :)

@andriikorotkov
Copy link
Contributor Author

andriikorotkov commented Oct 21, 2022

@alafanechere, yes, you are right, I am trying to do this exact part from our documentation (1 step). Also, I have already created subtasks for each of the steps. Also, I seem to have added this pull request to the right task.

Link to the issue for the first step - 18230

Link to the issue that I turned into a small epic - 17915

@andriikorotkov andriikorotkov marked this pull request as ready for review October 21, 2022 16:30
@andriikorotkov andriikorotkov temporarily deployed to more-secrets October 21, 2022 16:43 Inactive
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Thanks for providing more description.
I think you need to format your branch to follow our linting rules:
./gradlew format. (Please install pre-commit to avoid these problem in the future: pre-commit intall)

Moreover the platform build is failing with this error:
org.opentest4j.AssertionFailedError: expected: <0.40.12.001> but was: <0.40.15.001>

How did you pick the version the migration will be run for?

@andriikorotkov andriikorotkov temporarily deployed to more-secrets October 24, 2022 14:42 Inactive
@andriikorotkov andriikorotkov temporarily deployed to more-secrets October 24, 2022 15:04 Inactive
@andriikorotkov andriikorotkov temporarily deployed to more-secrets October 24, 2022 15:45 Inactive
@evantahler
Copy link
Contributor

Adding @colesnodgrass and @jdpgrailsdev from the platform team for review as well

@andriikorotkov andriikorotkov temporarily deployed to more-secrets October 25, 2022 10:53 Inactive
@tuliren tuliren temporarily deployed to more-secrets November 4, 2022 17:05 Inactive
@tuliren tuliren temporarily deployed to more-secrets November 4, 2022 17:41 Inactive
@tuliren tuliren temporarily deployed to more-secrets November 4, 2022 18:35 Inactive
@tuliren tuliren temporarily deployed to more-secrets November 4, 2022 20:33 Inactive
@tuliren tuliren temporarily deployed to more-secrets November 4, 2022 21:03 Inactive
@tuliren tuliren changed the title Updated StandardDestinationDefinition.yaml Add normalization to destination definition and actor definition table Nov 4, 2022
@tuliren tuliren merged commit 350d544 into master Nov 4, 2022
@tuliren tuliren deleted the akorotkov/18230_update_StandardDestinationDefinition branch November 4, 2022 21:19
letiescanciano added a commit that referenced this pull request Nov 7, 2022
* master: (69 commits)
  🪟 🐛 Fix wrong geography dropdown type #19021
  SAT: basic read on full catalog when `test_strictness_level == high` (#18937)
  Unhide DynamoDB destination (#18994)
  Fixed tests for destination connectors (#19007)
  🐛 Source Facebook Marketing: handle FacebookBadObjectError (#18971)
  Edit multi-cloud docs (#18972)
  🪟 🎉 Load credits consumption separate (#18986)
  Bmoric/extract source api (#18944)
  Migrating InvalidCursorException -> ConfigErrorException  (#18995)
  🪟 🎨 Fix banner link color (#18978)
  Handling configuration exceptions in IntegrationRunner (#18989)
  Add new workspace api endpoint (#18983)
  Add normalization to destination definition and actor definition table (#18300)
  Fix oauth controller (#18981)
  Fix migration dev center schema dump by run db-specific initialization script (#18984)
  fix master build failure (#18982)
  cleanup: delete debezium 1-4-2 module (#18733)
  Remove unused job persistence methods. (#18952)
  Hash filenames of extracted CSS (#18976)
  Fix typo in source code comment DataDaog ==> Datadog (#18911)
  ...
letiescanciano added a commit that referenced this pull request Nov 7, 2022
* master: (73 commits)
  🪟 🐛 Fix wrong geography dropdown type #19021
  SAT: basic read on full catalog when `test_strictness_level == high` (#18937)
  Unhide DynamoDB destination (#18994)
  Fixed tests for destination connectors (#19007)
  🐛 Source Facebook Marketing: handle FacebookBadObjectError (#18971)
  Edit multi-cloud docs (#18972)
  🪟 🎉 Load credits consumption separate (#18986)
  Bmoric/extract source api (#18944)
  Migrating InvalidCursorException -> ConfigErrorException  (#18995)
  🪟 🎨 Fix banner link color (#18978)
  Handling configuration exceptions in IntegrationRunner (#18989)
  Add new workspace api endpoint (#18983)
  Add normalization to destination definition and actor definition table (#18300)
  Fix oauth controller (#18981)
  Fix migration dev center schema dump by run db-specific initialization script (#18984)
  fix master build failure (#18982)
  cleanup: delete debezium 1-4-2 module (#18733)
  Remove unused job persistence methods. (#18952)
  Hash filenames of extracted CSS (#18976)
  Fix typo in source code comment DataDaog ==> Datadog (#18911)
  ...
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.

Update StandardDestinationDefinition.yaml
5 participants