[
https://issues.apache.org/jira/browse/SOLR-5944?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15368165#comment-15368165
]
Hoss Man commented on SOLR-5944:
--------------------------------
I've not had a chance to look at the latest patch, but here's some comment
responses...
bq. Since commitWithin and overwrite was being set here, I thought this is an
appropriate place to set the prevVersion to the cmd
But there's a fundemental difference betwen params like {{commitWithin}} and
{{overwrite}} and the new {{prevVersion}} param...
{{commitWithin}} and {{overwrite}} are _client_ specified options specific to
the xml/javabin update format(s). The fact that they can be specified as
request params is an implementation detail of the xml/javabin formats that they
happen to have in common, but are not exclusively specifyied as params -- for
example the XMLLoader only uses the params as defaults, they can be psecified
on a per {{<add/>}} basis.
The new {{prevVersion}} param however is an implementation detail of DUP ...
DUP is the _only_ code that should have to know/care that {{prevVersion}} comes
from a request param.
bq. Yes, this was intentional, and I think it doesn't make any difference. If
an "id" isn't found in any of these maps, it would mean that the previous
update was committed and should be looked up in the index.
bq. I think we don't need to worry. Upon receiving a prevPointer=-1 by whoever
reads this LogPtr, it should be clear why it was -1: if the command's
{{flags|UpdateLog.UPDATE_INPLACE}} is set, then this command is an in-place
update whose previous update is in the index and not in the tlog; if that flag
is not set, it is not an in-place update at all, and don't bother about the
prevPointer value at all (which is -1 as a dummy value).
We should have a comment to these affects (literally we could just paste that
text directly into a comment) when declaring the prevPointer variable in this
method.
bq. ... This was needed because the lucene document that was originally being
returned had copy fields targets of id field, default fields, multiple Field
per field (due to FieldType.createFields()) etc., which are not needed for
in-place updates.
Hmm... that makes me wonder -- we should make sure we have a test case of doing
atomic updates on numeric dv fields which have copyfields to other numeric
fields. ie: lets make sure our "is this a candidate for inplace updates" takes
into acount that the value being updated might need copied to another field.
(in theory if both the source & dest of the copy field are single valued dv
only then we can still do the in place updated as long as the copyField
happens, but even if we don't have that extra bit of logic we need a test that
the udpates are happening consistently)
bq. I wasn't sure how to deal with inc for dates, so left dates out of this
for simplicity for now
Hmmm... is that really the relevant question though?
I'm not sure how the existing (non-inplace) atomic update code behaves if you
try to "inc" a date, but why does it matter for the
{{isSupportedForInPlaceUpdate}} method?
* if date "inc" is supported in the existing atomic update code, then whatever
that code path looks like (to compute the new value) it should be the same for
the new inplace update code.
* if date "inc" is _not_ supported in the existing atomic update code, then
whatever the error is should be the same in the new inplace update code
Either way, I don't see why {{isSupportedForInPlaceUpdate}} should care -- or
if it is going to care, then it should care about the details (ie: return false
for (dv only) date field w/ "inc", but true for (dv only) date field with "set")
bq. I think this is fatal, since if the doc was deleted, then there shouldn't
have been an attempt to resolve to a previous document by that id. I think this
should never be triggered.
let's put those details in a comment where this Exception is thrown ... or
better yet, try to incorporate it into the Exception msg?
bq. I'm inclined to keep it to Long/null instead of long/-1, since
versionInfo.getVersionFromIndex() is also Long/null
Ah, ok ... good point -- can we go ahead and add some javadocs to that method
as well making that clear?
bq. ... I've changed this to now use the UpdateShardHandler's httpClient.
Ok, cool ... Yeah, that probably makes more sense in general.
bq. Not sure what needs to be done more here
Yeah, sorry -- that was a vague comment that even i don't know what i ment by,
was probably ment to be part of the note about the switch statemenet default.
> Support updates of numeric DocValues
> ------------------------------------
>
> Key: SOLR-5944
> URL: https://issues.apache.org/jira/browse/SOLR-5944
> Project: Solr
> Issue Type: New Feature
> Reporter: Ishan Chattopadhyaya
> Assignee: Shalin Shekhar Mangar
> Attachments: DUP.patch, SOLR-5944.patch, SOLR-5944.patch,
> SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
> SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
> SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
> SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
> SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
> SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
> SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
> SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
> SOLR-5944.patch, SOLR-5944.patch, SOLR-5944.patch,
> TestStressInPlaceUpdates.eb044ac71.beast-167-failure.stdout.txt,
> TestStressInPlaceUpdates.eb044ac71.beast-587-failure.stdout.txt,
> TestStressInPlaceUpdates.eb044ac71.failures.tar.gz,
> hoss.62D328FA1DEA57FD.fail.txt, hoss.62D328FA1DEA57FD.fail2.txt,
> hoss.62D328FA1DEA57FD.fail3.txt, hoss.D768DD9443A98DC.fail.txt,
> hoss.D768DD9443A98DC.pass.txt
>
>
> LUCENE-5189 introduced support for updates to numeric docvalues. It would be
> really nice to have Solr support this.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
