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

[PP-1956] Ensure the context transaction manager is being used for cr… #2181

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

dbernstein
Copy link
Contributor

…eating identifiers in SweepMonitor runs.

Description

This PR attempts to fix a problem we are seeing in production where the database session associated with the identifier object (obtained via the Session.object_session() method ) is sometimes not the same as the one associated with the context manager when the sweep monitor is running. I'm not sure that this will fix it, but hopefully is will get us closer to a fix for the sweep monitors.

Motivation and Context

https://ebce-lyrasis.atlassian.net/browse/PP-1956

How Has This Been Tested?

Unit tests still pass.

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@dbernstein dbernstein requested a review from a team November 20, 2024 23:59
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.02%. Comparing base (1d1b4eb) to head (199fc5d).
Report is 27 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2181   +/-   ##
=======================================
  Coverage   91.01%   91.02%           
=======================================
  Files         361      361           
  Lines       41205    41209    +4     
  Branches     8849     8851    +2     
=======================================
+ Hits        37504    37509    +5     
+ Misses       2425     2424    -1     
  Partials     1276     1276           

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

@jonathangreen
Copy link
Member

@dbernstein why do think that we are getting the wrong session here? I'm trying to understand how that could happen.

@jonathangreen
Copy link
Member

This one has been going for a LONG time at this point, and it seems like we are adding fixes on top of other fixes to try to get this sorted out, but it hasn't been working, I'm not sure I'm comfortable adding another fix here without understanding why this is happening.

This issue is related to at least these two other PRs: #1948 (PP-1491) & #2091 (PP-1755). Perhaps we need to go back to the original problem, and see if there is a better approach that we can take there so we get this actually sorted.

Copy link
Member

@jonathangreen jonathangreen left a comment

Choose a reason for hiding this comment

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

@dbernstein i approved this one, because I don't think its going to cause any issues, but I also don't see how it will help the problem here.

@dbernstein
Copy link
Contributor Author

@jonathangreen : My thinking here is that at least we're ensuring that the session that is being used by the identifier class to add a link is the same same that is being used by the context manager. Unless I have fundamentally misunderstood something here.

@dbernstein dbernstein merged commit 20b157f into main Dec 16, 2024
21 checks passed
@dbernstein dbernstein deleted the PP-1956-transaction-errors-in-sweep-monitor branch December 16, 2024 21:59
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