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

DM-46363: Inject SqlQueryContext into ObsCoreManager at top level #1081

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

dhirving
Copy link
Contributor

@dhirving dhirving commented Sep 16, 2024

Modified the ExposureRegionFactory interface to no longer expose SqlQueryContext outside daf_butler.

The ExposureRegionFactory interface has one internal implementation and one external implementation in the dax_obscore package. The internal implementation needs privileged access to SqlRegistry internals, but the external one does not. The external one now needs to support RemoteButler and can no longer provide a SqlRegistry object.

In order to make this change, the SqlQueryContext is now created once when ObscoreLiveTableManager is created and passed to the internal ExposureRegionFactory's constructor, instead of being part of the ExposureRegionFactory method call interface.

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes
  • (if changing dimensions.yaml) make a copy of dimensions.yaml in configs/old_dimensions

Modified the ExposureRegionFactory interface to no longer expose SqlQueryContext outside daf_butler.

The ExposureRegionFactory interface has one internal implementation and one external implementation in the dax_obscore package.  The internal implementation needs privileged access to SqlRegistry internals, but the external one does not.  The external one now needs to support RemoteButler and can no longer provide a SqlRegistry object.

In order to make this change, the SqlQueryContext is now created once when ObscoreLiveTableManager is created and passed to the internal ExposureRegionFactory's constructor, instead of being part of the ExposureRegionFactory method call interface.
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.69%. Comparing base (910bec6) to head (4627fb8).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1081   +/-   ##
=======================================
  Coverage   89.69%   89.69%           
=======================================
  Files         360      360           
  Lines       47059    47061    +2     
  Branches     9661     9661           
=======================================
+ Hits        42211    42213    +2     
  Misses       3480     3480           
  Partials     1368     1368           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dhirving dhirving marked this pull request as ready for review September 16, 2024 22:31
Copy link
Contributor

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

Looks great.

@dhirving dhirving merged commit ec1fe6a into main Sep 17, 2024
18 checks passed
@dhirving dhirving deleted the tickets/DM-46363 branch September 17, 2024 17:27
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