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

Refactor persistence strategy for dlo generation #292

Merged
merged 3 commits into from
Mar 10, 2025

Conversation

jiang95-dev
Copy link
Collaborator

@jiang95-dev jiang95-dev commented Mar 5, 2025

Summary

Made following changes:

  1. Add isPartitioned to the dlo_strategies table.
  2. Save partition level strategies to table properties.
  3. Change the modifier of appendToDloStrategiesTable so that the persistence strategy can be overridden.
  4. Stop generating partition level strategies for non-partitioned tables.

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

  • 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.

Added E2E test for DLO generation app logic.

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.

Copy link
Collaborator

@sumedhsakdeo sumedhsakdeo left a comment

Choose a reason for hiding this comment

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

Left a few comments

sumedhsakdeo
sumedhsakdeo previously approved these changes Mar 10, 2025
Copy link
Collaborator

@sumedhsakdeo sumedhsakdeo left a comment

Choose a reason for hiding this comment

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

Mostly nits.

@sumedhsakdeo sumedhsakdeo merged commit 54c1179 into linkedin:main Mar 10, 2025
1 check passed
log.info(
"Generated {} strategies {}",
strategies.size(),
strategies.stream().map(Object::toString).collect(Collectors.joining(", ")));
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you preserve this logging line, it's helpful to see the values for an app run

@jiang95-dev jiang95-dev mentioned this pull request Mar 17, 2025
17 tasks
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