[ 
https://issues.apache.org/jira/browse/SOLR-11198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16115470#comment-16115470
 ] 

Erick Erickson edited comment on SOLR-11198 at 8/5/17 5:43 PM:
---------------------------------------------------------------

WHOA! I got all confused between Isabelle's original comment and Cassandra's, 
Isabelle uses ZkCLI and Cassandra uses bin/solr.

I think the problem may be common to both, but my comments below are about the 
bin/solr version.

I think I see the problem and that this particular issue should be fixed, i.e. 
empty znodes with no children should be files locally rather than directories.

I don't think I want to try anything fancier. If a user copies down from ZK and 
the ZK state changes (i.e. a node with data gets children etc.) then erroring 
out and recommending that they use a clean local directory seems reasonable.

The rest of this is details you can read through if you want to or wonder what 
the heck the second point means.

I think the problem is in ZkMaintenanceUtils.downloadFromZk. This code
{code}
   if (children.size() == 0) {
        // If we didn't copy data down, then we also didn't create the file. 
But we still need a marker on the local
        // disk so create a dir.
        if (copyDataDown(zkClient, zkPath, file.toFile()) == 0) {
          Files.createDirectories(file);
        }
{code}

copyDataDown returns, well, zero if there's no data in the znode. I changed 
this code a while back because recursive copy wasn't working correctly 
(SOLR-10108) and I suspect it was introduced then. And it went in Solr 6.6 so 
it fits Isabelle's testing, including the bit about putting a comment in the 
stopwords.txt makes it a file rather than a directory..

zNodes can have both data and children. The logic says, in effect, "if the 
znode has no data and no children it'll be mapped into an empty directory". 
There's logic in there that if a znode has both data and children, it's made 
into a directory with a special file containing its data (zknode.data).

The simple fix would be just to decide the other way, i.e. a znode with no data 
and no children would become an empty file locally rather than a directory. 
That actually seems OK since I don't think ZK cares. 

The more I think about this the more I think the correct behavior is the easy 
fix above. ZK doesn't care; a znode can have data added and children added at 
will so if we make the local node an empty text file it'd be copied back up as 
a znode just like any other, albeit one without data or children, but that's OK 
as far as ZK is concerned. That doesn't preclude someone adding children to the 
ZK node after pushing it back up via another mechanism.

There'll still be an edge case where 
- someone copies an empty znode from ZK and it becomes a file
- the ZK node gets children
- the person tries the copy again from ZK to local

Since the copy down now already has a text file for that znode and then tries 
to make a directory there it'll probably error out. At least it better. I think 
we can live with that, it would be a good thing to add a test though.

I'm torn about whether to try to "do the right thing" in the case above. First, 
it appears to much of an edge case. One could write something like:
if (the local file is zero length) {
  remove the file
  create a directory in it's place
}

Which seems relatively safe. Except.... that doesn't handle the case where 
- a znode exists with data
- downconfig copies it locally as a text file
- the znode gets children through some other mechanism
- downconfig is run again

In that case "the right thing" would be to move the data to the zknode.data and 
make the local node into a directory and continue. Probably could do this "en 
passant" by just deleting the text file and continuing, the directory would be 
add it in its place and children would be added. Deleting data on the local 
disk makes me nervous though.

I'll assign it to myself, if anyone else wants to take it feel free.


was (Author: erickerickson):

I think I see the problem and that this particular issue should be fixed, i.e. 
empty znodes with no children should be files locally rather than directories.

I don't think I want to try anything fancier. If a user copies down from ZK and 
the ZK state changes (i.e. a node with data gets children etc.) then erroring 
out and recommending that they use a clean local directory seems reasonable.

The rest of this is details you can read through if you want to or wonder what 
the heck the second point means.

I think the problem is in ZkMaintenanceUtils.downloadFromZk. This code
{code}
   if (children.size() == 0) {
        // If we didn't copy data down, then we also didn't create the file. 
But we still need a marker on the local
        // disk so create a dir.
        if (copyDataDown(zkClient, zkPath, file.toFile()) == 0) {
          Files.createDirectories(file);
        }
{code}

copyDataDown returns, well, zero if there's no data in the znode. I changed 
this code a while back because recursive copy wasn't working correctly 
(SOLR-10108) and I suspect it was introduced then. And it went in Solr 6.6 so 
it fits Isabelle's testing, including the bit about putting a comment in the 
stopwords.txt makes it a file rather than a directory..

zNodes can have both data and children. The logic says, in effect, "if the 
znode has no data and no children it'll be mapped into an empty directory". 
There's logic in there that if a znode has both data and children, it's made 
into a directory with a special file containing its data (zknode.data).

The simple fix would be just to decide the other way, i.e. a znode with no data 
and no children would become an empty file locally rather than a directory. 
That actually seems OK since I don't think ZK cares. 

The more I think about this the more I think the correct behavior is the easy 
fix above. ZK doesn't care; a znode can have data added and children added at 
will so if we make the local node an empty text file it'd be copied back up as 
a znode just like any other, albeit one without data or children, but that's OK 
as far as ZK is concerned. That doesn't preclude someone adding children to the 
ZK node after pushing it back up via another mechanism.

There'll still be an edge case where 
- someone copies an empty znode from ZK and it becomes a file
- the ZK node gets children
- the person tries the copy again from ZK to local

Since the copy down now already has a text file for that znode and then tries 
to make a directory there it'll probably error out. At least it better. I think 
we can live with that, it would be a good thing to add a test though.

I'm torn about whether to try to "do the right thing" in the case above. First, 
it appears to much of an edge case. One could write something like:
if (the local file is zero length) {
  remove the file
  create a directory in it's place
}

Which seems relatively safe. Except.... that doesn't handle the case where 
- a znode exists with data
- downconfig copies it locally as a text file
- the znode gets children through some other mechanism
- downconfig is run again

In that case "the right thing" would be to move the data to the zknode.data and 
make the local node into a directory and continue. Probably could do this "en 
passant" by just deleting the text file and continuing, the directory would be 
add it in its place and children would be added. Deleting data on the local 
disk makes me nervous though.

I'll assign it to myself, if anyone else wants to take it feel free.

> downconfig downloads empty file as folder
> -----------------------------------------
>
>                 Key: SOLR-11198
>                 URL: https://issues.apache.org/jira/browse/SOLR-11198
>             Project: Solr
>          Issue Type: Bug
>      Security Level: Public(Default Security Level. Issues are Public) 
>    Affects Versions: 6.6
>         Environment: Windows 7
>            Reporter: Isabelle Giguere
>            Assignee: Erick Erickson
>            Priority: Minor
>
> With Solr 6.6.0, when downloading a config from Zookeeper (3.4.10), if a file 
> is empty, it is downloaded as a folder (on Windows, at least).
> A Zookeeper browser (Eclipse: Zookeeper Explorer) shows the file as a file, 
> however, in ZK.
> Noticed because we keep an empty synonyms.txt file in the Solr config 
> provided with our product, in case a client would want to use it.
> The workaround is simple, since the file allows comments: just add a comment, 
> so it is not empty.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org

Reply via email to