SOLR-18101: SolrJ commit options, fluent API#4109
SOLR-18101: SolrJ commit options, fluent API#4109dsmiley wants to merge 1 commit intoapache:mainfrom
Conversation
This is a little bit of an API refresh on UpdateRequest/AbstractUpdateRequest to make it clearer how to send various commit options, and their relation to optimize & expungeDeletes.
|
This is a draft, not using the API just showing what I think it should be and what I want to deprecate. |
|
Seems fine.. I'll be honest, I don't think I have strong opinons around this area... I almost never use SolrJ in my real world work, I only ever interact with it inside of Solr... I'm mostly in favour of anything that moves us forward, gets rid of waitFlush, and let's me remove more deprecated code in main. I may also just be a bit fatigued around all the change/discussion/work in the Java SolrCLient world ;-)! Not very helpful, I know. |
gerlowskija
left a comment
gerlowskija
left a comment
There was a problem hiding this comment.
Looks great at this stage. I left a few real nit-picky comments on naming etc. that you can feel free to ignore, but the overall enum CommitOptions direction is a strongly positive step IMO 👍
I can already see @epugh licking his chops at all those 'setAction' deprecations 😛
| public static final EnumSet<CommitOption> DEFAULTS = EnumSet.of(waitSearcher, openSearcher); | ||
| } | ||
|
|
||
| @Deprecated |
|
|
||
| public enum CommitOption { | ||
| /** Makes the changes visible. True by default. */ | ||
| openSearcher, |
There was a problem hiding this comment.
[0] Don't care strongly but I'm used to seeing enum values as ALL_CAPS, and I imagine most readers are as well.
There was a problem hiding this comment.
Yeah, I like the all caps as well, and we should probably have a lint rule for it!
There was a problem hiding this comment.
I confess I'm annoyed with the collective set of people (whoever they are; I don't care) that started making all-caps a defacto convention standard. Do they get paid by the lines of code they write? Do we need silly conversion methods between the all-caps and corresponding natural params they are supposed to map to?
There was a problem hiding this comment.
To be clear, the very explicit decision I made to use this casing is to align with the parameter names they correspond to.
| super(m, path, SolrRequestType.UPDATE); | ||
| } | ||
|
|
||
| public enum CommitOption { |
There was a problem hiding this comment.
[0] This is well out-of-scope for your PR since you're focused on how to make the Java API better given the HTTP APIs that already exist, but...
Reading this PR really draws my attention to how quirky our HTTP-level API here is. Specifically the main decision conceptually when making a commit is "hard or soft?"...this HTTP API obscures that entirely. You could read these params and not even grok that distinction.
I wonder if it'd be worth adding some sort of commit.type query param that accepts values hard, soft, or both.
Just thinking aloud...
| softCommit; | ||
|
|
||
| /** Default standard set of commit options: waitSearcher and openSearcher */ | ||
| public static final EnumSet<CommitOption> DEFAULTS = EnumSet.of(waitSearcher, openSearcher); |
There was a problem hiding this comment.
[0] These aren't just general update "defaults", they're defaults specifically around committing. Might be good to represent that in the name.
There was a problem hiding this comment.
This constant is scoped inside the CommitOption, thus CommitOption.DEFAULTS.
| return this; | ||
| } | ||
|
|
||
| public AbstractUpdateRequest commitAndOptimize(EnumSet<CommitOption> options, int maxSegments) { |
There was a problem hiding this comment.
[0] Do we really need the commitAnd in the name of this method and commitAndExpungeDeletes below? It feels like a bit of an implementation detail that we're also setting the commit flag - the expunge or the optimize are the main thing by far.
There was a problem hiding this comment.
It reflects the reality of what's happening. I recognize the "optimize" or "expungeDeletes" are a bigger impact. I wanted the API here to reflect something that was previously non-obvious.
| return deleteById(id, route, null); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
[+1] Really appreciate the Javadocs you're adding here!
| params.set(UpdateParams.COMMIT, true); | ||
| // only set if varies from default | ||
| if (options.contains(CommitOption.softCommit) == true) { | ||
| params.set(UpdateParams.SOFT_COMMIT, true); |
There was a problem hiding this comment.
speaking of enum capitalization... my choice allows me to skip referencing UpdateParams constants and instead take the enum name itself as-is. I didn't do that... maybe I should. OTH, I often do a find-usages on canonical constants to see where we reference them.
3 participants
This is a little bit of an API refresh on UpdateRequest/AbstractUpdateRequest to make it clearer how to send various commit options, and their relation to optimize & expungeDeletes.
https://issues.apache.org/jira/browse/SOLR-18101