[GitHub] zookeeper pull request #624: ZOOKEEPER-3108:use a new property server.id in ...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/624#discussion_r217873847
  
--- Diff: zookeeper-docs/src/documentation/content/xdocs/zookeeperAdmin.xml 
---
@@ -265,18 +266,25 @@ server.3=zoo3:2888:3888
   role="bold">server.id=host:port:port. The parameters 
host and port are straightforward. You attribute 
the
-  server id to each machine by creating a file named
+  server id to each machine by setting server.id
+  to a unique integer for each zookeeper server.To keep backwards 
compatibility,
+  you can still creat a file named
   myid, one for each server, which resides in
   that server's data directory, as specified by the configuration 
file
-  parameter dataDir.
+  parameter dataDir.If the unique 
id is both set in the
+  server.id of zoo.cfg and myid file,the server.id has the priority
--- End diff --

i think we should return an error in this case. this will make debugging 
problems that result from this situation hard.


---

[GitHub] zookeeper pull request #624: ZOOKEEPER-3108:use a new property server.id in ...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/624#discussion_r217873955
  
--- Diff: zookeeper-docs/src/documentation/content/xdocs/zookeeperAdmin.xml 
---
@@ -265,18 +266,25 @@ server.3=zoo3:2888:3888
   role="bold">server.id=host:port:port. The parameters 
host and port are straightforward. You attribute 
the
-  server id to each machine by creating a file named
+  server id to each machine by setting server.id
+  to a unique integer for each zookeeper server.To keep backwards 
compatibility,
+  you can still creat a file named
--- End diff --

we should also talk about the importance of not changing the id since it 
becomes much easier with the configuration file option. we should have 
documented that anyway, but it becomes much more important with this change 
since the id is decoupled from the data.


---

[GitHub] zookeeper pull request #624: ZOOKEEPER-3108:use a new property server.id in ...

[enixon]

Github user enixon commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/624#discussion_r217812851
  
--- Diff: zookeeper-docs/src/documentation/content/xdocs/zookeeperAdmin.xml 
---
@@ -265,18 +266,25 @@ server.3=zoo3:2888:3888
   role="bold">server.id=host:port:port. The parameters 
host and port are straightforward. You attribute 
the
-  server id to each machine by creating a file named
+  server id to each machine by setting server.id
+  to a unique integer for each zookeeper server.To keep backwards 
compatibility,
+  you can still creat a file named
--- End diff --

nit - spelling 'create'


---

[GitHub] zookeeper pull request #624: ZOOKEEPER-3108:use a new property server.id in ...

[eolivelli]

Github user eolivelli commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/624#discussion_r217763082
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/flexible/QuorumMaj.java ---
@@ -83,7 +83,7 @@ public QuorumMaj(Properties props) throws ConfigException 
{
 String key = entry.getKey().toString();
 String value = entry.getValue().toString();
 
-if (key.startsWith("server.")) {
+if (key.startsWith("server.") && !key.startsWith("server.id")) 
{
--- End diff --

Maybe key.equals(server.id)


---

[GitHub] zookeeper pull request #624: ZOOKEEPER-3108:use a new property server.id in ...

[maoling]

GitHub user maoling opened a pull request:

https://github.com/apache/zookeeper/pull/624

ZOOKEEPER-3108:use a new property server.id in the zoo.cfg to substitute 
for myid file

- use a new property `server.id` in the `zoo.cfg` to substitute for `myid 
`file,If the unique id is both set in the `server.id` of `zoo.cfg` and `myid` 
file,the `server.id` has the priority.
- I had tested this patch in my docker in standalone and qurom mode with 
mixed `myid` file and the `server.id`
  it has no intrude,totally backwards compatibility,branch3.4 can still use 
myid file, but branch 3.5+,
  suggest to use `server.id `in the zoo.cfg
- the related UTs `ZooKeeperServerMainTest.testStandalone()` and 
`QuorumPeerMainTest.testQuorum() `have passed,before reviewing,let's listen to 
the QA report in case of something I miss 
- more details and disscusion in 
[ZOOKEEPER-3108](https://issues.apache.org/jira/browse/ZOOKEEPER-3108) 
- Let me list why this is needed:
**1.** put the `myid` which's about the conf of server in the `dataDir` is 
not a good idea.if I want to `rm -rf `the data in this directory manually,I 
will delete the `myid` file by mistake.
**2.** conf like this will be easy-to-read,and tell me who I am just like 
`kafka` does.
  ``` 
  server.id = 2
  ---***---
  server.1=172.17.0.2:2888:3888
  server.2=172.17.0.2:12888:13888
  server.3=172.17.0.2:22888:23888
```



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/maoling/zookeeper ZOOKEEPER-3108

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/624.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #624


commit af2a12c6e4037cf0010ea15c43355ec4f5ea4318
Author: maoling 
Date:   2018-09-05T08:44:54Z

ZOOKEEPER-3108:use a new property server.id in the zoo.cfg to substitute 
for myid file




---