Skip to content

Conversation

@koentsje
Copy link
Member

@koentsje koentsje commented Feb 27, 2025


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Feb 27, 2025

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@koentsje koentsje requested a review from sebersole February 27, 2025 13:03
final SqmPolymorphicRootDescriptor<R> unmappedPolymorphicDescriptor = (SqmPolymorphicRootDescriptor<R>) unmappedPolymorphicReference.getReferencedPathSource();
final Set<EntityDomainType<? extends R>> implementors = unmappedPolymorphicDescriptor.getImplementors();
final SqmPolymorphicRootDescriptor<?> unmappedPolymorphicDescriptor = (SqmPolymorphicRootDescriptor<?>) unmappedPolymorphicReference.getReferencedPathSource();
final Set<EntityDomainType<?>> implementors = unmappedPolymorphicDescriptor.getImplementors();
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn’t say that we need to roll back such trivial things.

Copy link
Member

@sebersole sebersole Mar 1, 2025

Choose a reason for hiding this comment

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

To clarify, I would not necessarily say it is trivial. But I do think it is questionable whether the change is obvious and self-evident. I think given the request, this is how any of us would have implemented it.

And in fact, @beikov contributed much to the actual implementation - #7468 (comment)

Let's roll back this removal

@sebersole
Copy link
Member

sebersole commented Mar 1, 2025

HHH-12921: Remove commit 6911efd by Carlos Aristu carlos.aristu@openbravo.com

Really, this change is simply marking the validation threads as daemons. The code had already used threads for this purpose. I think that marking those threads as daemons is self-evident.

So to me, this becomes a question of whether we would have made changes differently. After numerous web searches, the pattern used here is exactly the approach recommended in places like Stackoverflow and blogs - so I'd have to say no, this is a non-novel solution.

@sebersole
Copy link
Member

@koentsje You want to rework the PR per those comments? I can if you prefer, just let me know.

Copy link
Member

@sebersole sebersole left a comment

Choose a reason for hiding this comment

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

See comments for requested changes

@sebersole
Copy link
Member

sebersole commented Mar 2, 2025

Honestly, I'm even inclined to call 50c0add trivial and self-evident as well - both main and tests.

Long story short, I think I'm inclined to just keep 330d2fb from here.

WDYT?

@sebersole
Copy link
Member

sebersole commented Mar 2, 2025

See #9826 for alternative keeping just the 330d2fb commit from here which removes

@sebersole
Copy link
Member

Going with my PR for now as I have not heard back and want to get this moving along. We can looking at cherry-picking any of these other removals later if we deem it necessary

@sebersole sebersole closed this Mar 3, 2025
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