Configuration structure improvements in the Hibernate Search extension #5203
Conversation
yrodiere
force-pushed
the
hsearch-doc-structure
branch
3 times, most recently
from
aeb3d81 to
ee79a83
Compare
…ibernate Search extension config For consistency, because we already do this for: * default-backend * background-failure-handler So we may as well do it for these, too: * query-loading.cache-lookup.strategy * query-loading.fetch-size * automatic-indexing.synchronization.strategy * automatic-indexing.enable-dirty-check
…n in the Hibernate Search extension docs
… section in the Hibernate Search extension config Because having it at the very top of the property list in the documentation is quite confusing, considering it's really not something the average user will need.
… Search configuration For consistency with the runtime configuration.
yrodiere
force-pushed
the
hsearch-doc-structure
branch
from
ee79a83 to
973815d
Compare
|
Rebased on master just now. |
|
Please let me review the Search PRs before merging them. |
| quarkus.hibernate-search.elasticsearch.index-defaults.lifecycle.required-status=yellow <9> | ||
| quarkus.hibernate-search.elasticsearch.index-defaults.lifecycle.strategy=drop-and-create <7> | ||
| quarkus.hibernate-search.elasticsearch.index-defaults.lifecycle.required-status=yellow <8> | ||
| quarkus.hibernate-search.automatic-indexing.synchronization.strategy=searchable <9> |
There was a problem hiding this comment.
TBH, I'm not totally sold on this change. It makes the configuration all weird.
I'm also surprised it's not something you can configure per backend in Search.
There was a problem hiding this comment.
Synchronization is about:
- Whether to force a refresh on the backend side before considering indexing "done" (
committedvs.searchable). - Whether we wait for indexing to be "done" after a transaction, or we just proceed without one works are queued (
queuedvs.committed/searchable).
Number 1 could theoretically be configured per backend, or even per index, though I don't really see the point.
Number 2 definitely cannot.
Maybe at some point we could split that into two separate configuration options, number 1 being configurable per backend while number 2 would not. However, that would introduce nonsensical combinations (do not wait for indexing to be "done", but force a refresh before considering it's "done"...). I also think it would make it even harder to explain the whole concept, but maybe that's just me.
There was a problem hiding this comment.
What bugs me a bit tbh is having this property outside of the elasticsearch context.
The other ones we had there were for advanced configurations. Here the default configuration looks a bit asymmetrical.
|
This is a breaking change, we should be careful about that, I removed the backport tag, I don't want to do that just after a CR. |
|
@gsmet the general attitude so far has been that breaking changes have to be backported before the final - not the opposite.
hu? now that's confusing |
|
You don't backport breaking changes between a CR and a final, or you do another CR. We don't plan to do another CR so... At least, that's how I learned to do things :).
why is it? This one has breaking changes that needs to be carefully thought out and I'm the one who worked on it initially. Having it merged silently without adding |
because the PR is sent to the team, and we're one team - everyone trying to help. Mind you, I'm happy to follow your suggestion but I suspect tomorrow someone else will merge this, trespassing your territory again. If you'd like to change the structure, I would support you. I already hinted at this: there's a lot of noise trying to follow ALL changes for everyone, I don't see that scaling. But also I regularly get my PRs reviewed (and even merged) by people out of the same expert zone; one needs to learn accepting that a PR is not a draft, it's a bitter-sweet situation as I'm also not happy that lots of stuff I hoped would be done differently got merged before I could even have look. It's a consequence of the current process; if you want to "own" all changes on an extension, you'll have to make this clear upfront as I can't see this stopping. Or ask Yoann to work with you on your own fork. |
|
When there is a breaking change, the maintainer of the extension should at least have a look at the PR. Another PR was merged on the HV extension while I was away without giving me a chance to review it while I explicitly asked to (I assigned it to me and posted a comment about it). The PR broke CI and then people are asking me to fix that. I'm not talking about simple/obvious bugfixes or documentation fixes. This PR breaks the compatibility for a configuration property that is probably used by all the persons using this extension (as it's strictly needed to have tests working reliably). And there was a child PR on the quickstarts that you didn't merge (quarkusio/quarkus-quickstarts#359) so it broke the quickstarts for several days and I was the one people pinged about it. I'm not asking for hard rules, just some common courtesy so that a person familiar with an extension has a look before merging something structuring. And this change is. Typically, I wouldn't merge anything not obvious on the Spring-compatibility extensions without asking Georgios to have a look first. |
Yes that's what I'm referring to. I'm concerned as well, and think we need to change the process. Comments on github won't do the trick, as you've seen on the other PRs. |
3 participants
Based on #5202 , which should be merged first.=> DonePlease have a look at the documentation before/after to make your mind about this changeset.
Run this:
And compare the structure of the documentation with what we already have here.