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

Use ContentManagerSession in GetMany #8705

Draft
wants to merge 5 commits into
base: 1.10.x
Choose a base branch
from

Conversation

MatteoPiovanelli-Laser
Copy link
Contributor

This is the first hacky attempt. I'm not satisfied with the code: I look at it and it feels like it's doing some stuff too many times where it shouldn't.
Anyway, it does reduce the number of repeated calls to the database. In some setups, that may become significant.

I haven't had a chance to properly test this, or measure the timing performance, to make sure that we aren't losing time in the work we do to avoid calls to the db.

To summarize, my concerns with my code in this PR right now:

  • it looks hacky to me.
  • I'm not 100% confident it will always 100% of the time in every condition return the exact same results as the live code, even though I can't think of a case where it wouldn't.
  • I have tested the timing of the methods I changed.

@MatteoPiovanelli-Laser
Copy link
Contributor Author

@BenedekFarkas forgot to tag you in the main post

@BenedekFarkas
Copy link
Member

I'll need a few days to get down to testing it properly, but it looks good on first glance!

@MatteoPiovanelli-Laser
Copy link
Contributor Author

@BenedekFarkas I just realized this is still open. My tests still look good, But I'm still not too fond of the code I wrote.

@BenedekFarkas
Copy link
Member

Well, DefaultContentManager is very old, so there's probably a lot of stuff we could refactor (which sort of affects what you can achieve there). I wouldn't merge it without unit/Spec testing completing on it first but being able to run tests is still blocked by me (#8686). We could also add some specific tests to verify this change, if necessary.

@MatteoPiovanelli-Laser
Copy link
Contributor Author

this is targeted at 1.10.x, so I think specflows should be able to run... I'll get back to you on this.

@MatteoPiovanelli-Laser
Copy link
Contributor Author

I merged the latest 1.10.x into the branch for this PR and I was able to run all specflows.

@MatteoPiovanelli-Laser
Copy link
Contributor Author

MatteoPiovanelli-Laser commented Mar 12, 2024

I'm still not 100% on this solution.
Results look consistent, and I see fewer queries in the logs, but no great impact on final timings (in quick page load tests I did).
Perhaps doing a conditional and short circuiting the case when there is a single Id being queried would be useful, as that is an actual common case I see in our tenants, in ContentPickerFields and such

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants