Thanks for the KIP, Ismael, looks great. I have just a couple of comments and questions:
- I assume the znode containing the cluster id is persistent. Is there ever the need to delete that znode to force a new instance id, and if so, how is it expected to happen? I'm thinking about a scenario in which I have a cluster, I wipe out intentionally to reset my state, and I want to start with a fresh instance of the cluster. In such a case, it should be as simple as deleting the znode manually, so I'm just wondering if this is what we are going to expect from users. The alternative is to assume that the zookeeper ensemble is also going to be wiped out in this case, or at least the corresponding sub-tree. - I'm not actually suggesting that we do things manually, but I can see some benefit in manually creating the cluster id znode and have brokers only check that the znode is there before starting. - One of the messages mentions changing names, which is a good point. If it isn't necessary for correctness that all brokers have either no id or the same id, then I'd think it is fine to just watch that znode for changes. Brokers that are connect to zk will receive eventually a notification. But, you point out that changing the name for a working cluster isn't necessary a desirable feature, so this might not be needed. Cheers, -Flavio > On 03 Sep 2016, at 08:46, sumit arrawatia <sumit.arrawa...@gmail.com> wrote: > > Hi Dong, > > If you set the cluster.id in the config, the problem is how you > change/update the cluster.id . > > > You will need to change the all the config files and make sure every one of > them is correct as well as update the ZK metadata. This will require a > reboot/downtime of the entire cluster, whereas generating ids (along with > the yet-to-be-published resource tags KIP ) makes it easy for the admins to > update the human readable name without reboots/ clusterwide changes to > config. > > > Also, when we implement the change to write cluster.id in meta.properties, > the process of updating the cluster.id becomes even more complicated. > > > 1. We will have to ensure that the entire process is transactional. It will > be painful from an operational standpoint as it would require as much > operational downtime and care as a version upgrade because it will modify > the on-disk data. The recovery/rollback scenario would be very difficult > and would probably need manual changes to meta.properties. > > > 2. Given that we would want to generate an error if the cluster.id in > meta.properties doesn't match cluster.id in ZK, we will have to setup > complicated logic in ZK to ensure we forgo the check when changing the > cluster.id. I am not even sure how to do it properly for a rolling upgrade > without downtime. > > > All these points are based on my experience running Elasticsearch in > production. ElasticSearch specifies cluster.name statically in the > properties as well as includes it in the data directory name and changing > it is a nightmare. > > > You would think that naming changes should be rare but in my experience > they are not. Sometimes typos creep in, sometimes you have to change the > name to consolidate clusters or divide them and sometimes the infra team > decides to change the deployment metadata. > > > This is why I think AWS approach of (assigning immutable ids to resources + > human readable names in tags) works very well operationally. > > > Hope this helps ! > > > Cheers, > > Sumit > > On Fri, Sep 2, 2016 at 11:44 PM, Dong Lin <lindon...@gmail.com> wrote: > >> Hey Ismael, >> >> Thanks for your reply. Please see my comment inline. >> >> On Fri, Sep 2, 2016 at 8:28 PM, Ismael Juma <ism...@juma.me.uk> wrote: >> >>> Hi Dong, >>> >>> Thanks for your feedback. Comments inline. >>> >>> On Thu, Sep 1, 2016 at 7:51 PM, Dong Lin <lindon...@gmail.com> wrote: >>>> >>>> I share the view with Harsha and would like to understand how the >> current >>>> approach of randomly generating cluster.id compares with the approach >> of >>>> manually specifying it in meta.properties. >>>> >>> >>> Harsha's suggestion in the thread was to store the generated id in >>> meta.properties, not to manually specify it via meta.properties. >>> >>>> >>>> I think one big advantage of defining it manually in zookeeper is that >> we >>>> can easily tell which cluster it is by simply looking at the sensor >> name, >>>> which makes it more useful to the auditing or monitoring use-case that >>> this >>>> KIP intends to address. >>> >>> >>> If you really want to customise the name, it is possible with the current >>> proposal: save the appropriate znode in ZooKeeper before a broker >>> auto-generates it. We don't encourage that because once you have a >>> meaningful name, there's a good chance that you may want to change it in >>> the future. And things break down at that point. That's why we prefer >>> having a generated, unique and immutable id complemented by a changeable >>> human readable name. As described in the KIP, we think the latter can be >>> achieved more generally via resource tags (which will be a separate KIP). >>> >>> Can you elaborate what will break down if we need to change the name? >> >> Even if we can not change name because something will breakdown in that >> case, it seems that it is still better to read id from config than using a >> randomly generated ID. In my suggested solution user can simply choose not >> to change the name and make sure there is unique id per cluster. In your >> proposal you need to store the old cluster.id and manually restore it in >> zookeeper in some scenarios. What do you think? >> >> >>>> On the other hand, if you can only tell whether two >>>> sensors are measuring the same cluster or not. Also note that even this >>>> goal is not easily guaranteed, because you need an external mechanism >> to >>>> manually re-generate znode with the old cluster.id if znode is deleted >>> or >>>> if the same cluster (w.r.t purpose) is changed to use a different >>>> zookeeper. >>>> >>> >>> If we assume that znodes can be deleted at random, the cluster id is >>> probably the least of one's worries. And yes, when moving to a >>> different ZooKeeper while wanting to retain the cluster id, you would >> have >>> to set the znode manually. This doesn't seem too onerous compared to the >>> other work you will have to do for this scenario. >>> >>> Maybe this work is not much compared to other work. But we can agree that >> no work is better than little work, right? I am interested to see if we can >> avoid the work and still meet the motivation and goals of this KIP. >> >> >>>> I read your reply to Harsha but still I don't fully understand your >>> concern >>>> with that approach. I think the broker can simply register group.id in >>>> that >>>> znode if it is not specified yet, in the same way that this KIP >> proposes >>> to >>>> do it, right? Can you please elaborate more about your concern with >> this >>>> approach? >>>> >>> >>> It's a bit difficult to answer this comment because it seems like the >>> intent of your suggestion is different than Harsha's. >>> >>> I am not necessarily opposed to storing the cluster id in meta.properties >>> (note that we have one meta.properties per log.dir), but I think there >> are >>> a number of things that need to be discussed and I don't think we need to >>> block KIP-78 while that takes place. Delivering features incrementally >> is a >>> good thing in my opinion (KIP-31/32, KIP-33 and KIP-79 is a good recent >>> example). >>> >> >> If I understand it right, the motivation of this KIP is to allow cluster to >> be uniquely identified. This is a useful feature and I am not asking for >> anything beyond this scope. It is just that reading cluster.id from config >> seems to be a better solution in order to meet the motivation and all the >> goals described in the KIP. More specifically, using cluster.id not only >> allows user to distinguish between different clusters, it also lets user >> identify cluster. In comparison, randomly generated cluster.id allows user >> to distinguish cluster with a little bit more effort, and doesn't allow >> user to identify a cluster by simply reading e.g. sensor name. Did I miss >> something here? >> >> >>> >>> Ismael >>> >>> P.S. For what is worth, the following version of the KIP includes an >>> incomplete description (it assumes a single meta.properties, but there >>> could be many) of what the broker would have to do if we wanted to save >> to >>> meta.properties and potentially restore the znode from it. The state >> space >>> becomes a lot more complex, increasing potential for bugs (we had a few >> for >>> generated broker ids). In contrast, the current proposal is very simple >> and >>> doesn't prevent us from introducing the additional functionality later. >>> >>> https://cwiki.apache.org/confluence/pages/viewpage. >> action?pageId=65868433 >>> >> >> IMO reading cluster.id from config should be as easy as reading broker id >> from config. Storing cluster.id from config in znode requires the same >> amount of effort as storing randomly generated cluster.id in znode. Maybe >> I >> missed something here. Can you point me to the section of the KIP that >> explains why it is more difficult if we want to read cluster.id from >> config? >> > > > > -- > Regards, > Sumit
