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

Adding table properties to signal replication run state #283

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rohitkum2506
Copy link
Collaborator

@rohitkum2506 rohitkum2506 commented Jan 30, 2025

Summary

Adding a property in tblProperties to indicate replication run state for a table based on replicationConfig addition/update.
Property: replication.enableSetup
Cases:

  1. Table Created without replication Policy -> Replication.enableSetup is not added.
  2. Table Created with replication Policy -> Replication.enableSetup is added. Values is set to true
  3. Table is updated with a new value for replication policy -> Replication.enableSetup is added. Value is set to true
  4. Table is updated without changes to replication policy - > No changes to replication.enableSetup property

Based on boolean value of the property, replication job is triggered only when needed, ie. when policy is added or updated.

Changes

  • Client-facing API Changes
  • Internal API Changes
  • Bug Fixes
  • New Features
  • Performance Improvements
  • Code Style
  • Refactoring
  • Documentation
  • Tests

For all the boxes checked, please include additional details of the changes made in this pull request.

Testing Done

Unit tests added.
Local Docker system tests:
1: Created table without replication config -> Observed tblProperty is not set
2. Created table without replication config -> tblProperty is set to true
3. Updated tblProperty to false
4. Updated replication config -> tblProperty Replication.enableSetup was set to true
5. Added other table policies -> no effect on tblProperty Replication.enableSetup

Screenshot 2025-01-29 at 10 50 21 PM Screenshot 2025-01-29 at 10 59 26 PM Screenshot 2025-01-29 at 10 59 37 PM
  • Manually Tested on local docker setup. Please include commands ran, and their output.
  • Added new tests for the changes made.
  • Updated existing tests to reflect the changes made.
  • No tests added or updated. Please explain why. If unsure, please feel free to ask for help.
  • Some other form of testing like staging or soak time in production. Please explain.

For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request.

Additional Information

  • Breaking Changes
  • Deprecations
  • Large PR broken into smaller PRs, and PR plan linked in the description.

For all the boxes checked, include additional details of the changes made in this pull request.

@rohitkum2506 rohitkum2506 marked this pull request as ready for review January 30, 2025 07:06
@@ -26,6 +26,7 @@
public final class InternalRepositoryUtils {

protected static final String POLICIES_KEY = "policies";
protected static final String REPLICATION_SETUP_KEY = "replication.enableSetup";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this is a leakage of implementation details to expose such internal state as table properties, not to mention the table table properties itself can be lost / modified by users directly. We can certainly make this a preserved key to defend against that argument, but I believe this is the wrong step to start to begin with.

Let's iterate on slack about what is the problem trying to solve here and understand that better.

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