Skip to content

Comments

SOLR-15209: Make the LegacyAssignStrategy the DefaultPlacementPlugin#512

Merged
HoustonPutman merged 7 commits intoapache:mainfrom
HoustonPutman:default-placement-plugin
Jan 13, 2022
Merged

SOLR-15209: Make the LegacyAssignStrategy the DefaultPlacementPlugin#512
HoustonPutman merged 7 commits intoapache:mainfrom
HoustonPutman:default-placement-plugin

Conversation

@HoustonPutman
Copy link
Contributor

https://issues.apache.org/jira/browse/SOLR-15209

There are some issues with each of the placementPlugins, but will follow up separately.

Copy link
Member

@murblanc murblanc left a comment

Choose a reason for hiding this comment

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

I see you're considering SOLR-15209 as making LegacyAssignStrategy the default placement, when it originally was (and still is in Jira) "Make the AffinityPlacementFactory the default placement plugin".

With this new goal of SOLR-15209 the PR seems reasonable, I've left a few comments.

The original goal of SOLR-15209 was to make the realistic placement strategy (affinity placement) the default one to 1. have better/more flexible placement overall, and 2. make sure this code is running (in installations, in tests) to find issues with it.

As it stands, tests still run against the legacy strategy and do not test the "real" placement plugin.

@HoustonPutman
Copy link
Contributor Author

I see you're considering SOLR-15209 as making LegacyAssignStrategy the default placement, when it originally was (and still is in Jira) "Make the AffinityPlacementFactory the default placement plugin".

Fair enough, I thought about creating a new ticket, which I still can. I ended up not because making the legacy strategy a plugin, like the rest, was a part of the description. If you would like a new JIRA to track this, so that SOLR-15209 can be used to track changing the default, I am happy to make the new one and link it 🙂

The original goal of SOLR-15209 was to make the realistic placement strategy (affinity placement) the default one to 1. have better/more flexible placement overall, and 2. make sure this code is running (in installations, in tests) to find issues with it.

We can always do this (even under a different ticket), then decide to change the default implementation separately. I'm perfectly fine with that as long as we test how the performance of cluster operations are affected.

But I definitely agree that there should be more tests around these. (I think while writing this, I found some issues that I will surface in new JIRAs)

@murblanc
Copy link
Member

I hope eventually we can make another placement plugin (the affinity placement one) the default placement, but I don't have time any time soon to look at the tests, understand them and make them work with it.
If you can leave a TODO or comment where the default is set that eventually we'd want another default, I think you can hijack the jira as planned (we'll create a new jira when/if we get to change the default).

@HoustonPutman
Copy link
Contributor Author

Ok I will then rename the DefaultPlacementPlugin as LegacyPlacementPlugin, since it would not make sense in the future if the DefaultPlacementPlugin is not the default.

I will leave a TODO comment for the future.

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