[
https://issues.apache.org/jira/browse/SOLR-5944?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15368262#comment-15368262
]
Ishan Chattopadhyaya commented on SOLR-5944:
--------------------------------------------
{quote}
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.
{quote}
Sure, it makes sense. I'll fix it.
bq. 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.
I had put this comment there:
{code}
@return If cmd is an in-place update, then returns the pointer (in the tlog) of
the previous update that the given update depends on. Returns -1 if this is not
an in-place update, or if we can't find a previous entry in the tlog.
{code}
But now I have updated it to make it even more detailed:
{code}
@return If cmd is an in-place update, then returns the pointer (in the tlog) of
the previous update that the given update depends on.
* Returns -1 if this is not an in-place update, or if we can't find a
previous entry in the tlog. Upon receiving a -1, 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).)
{code}
{quote}
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)
{quote}
Sure, I'll add such a test. The latest patch incorporates the behaviour you
suggested: if any of the copy field targets is not a in-place updateable field,
then the entire operation is not an in-place update (but a traditional atomic
update instead). But, if copy field targets of an updated field is also
supported for an updateable dv, then it is updated as well.
{quote}
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")
{quote}
For now I've removed date field totally out of scope of this patch. If there is
a update to date that is needed, it falls back to traditional atomic update. I
can try to deal with the trie date field, if you suggest.
bq. let's put those details in a comment where this Exception is thrown ... or
better yet, try to incorporate it into the Exception msg?
I had put this exception in the patch: {{Unable to resolve the last full doc in
tlog fully, and document not found in index even after opening new rt searcher.
}} but now I'll chang it to: {{Unable to resolve the last full doc in tlog
fully, and document not found in index even after opening new rt searcher. If
the doc was deleted, then there shouldn't have been an attempt to resolve to a
previous document by that id.}}
bq. Ah, ok ... good point – can we go ahead and add some javadocs to that
method as well making that clear?
Sure, I'll update the javadocs for that existing method as well.
> 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]
