Hi, >>>>The LeaseStartTimeMs is expected to be the broker's 'System.currentTimeMillis()' at the point of the request. The active controller will add its lease period to this in order >>>>to compute the LeaseEndTimeMs. I think this is a bit confusing. Monotonic clock on the active controller should be used to track leases. For example, https://issues.apache.org/jira/browse/ZOOKEEPER-1616 https://github.com/etcd-io/etcd/pull/6888/commits/e7f4010ccaf28b6ce64fe514d25a4b2fa459d114
Then we will not need LeaseStartTimeMs? Instead of LeaseStartTimeMs, can we call it LeaseTTL? The active controller can then calculate LeaseEndTime = System.nanoTime() + LeaseTTL. In this case we might just drop LeaseEndTimeMs from the response, as the broker already knows about the TTL and can send heartbeats at some fraction of TTL, say every TTL/4 milliseconds.(elapsed time on the broker measured by System.nanoTime) Thanks, Unmesh On Tue, Aug 4, 2020 at 4:48 AM Colin McCabe <cmcc...@apache.org> wrote: > On Mon, Aug 3, 2020, at 15:51, Jose Garcia Sancio wrote: > > Thanks for the KIP Colin, > > > > Here is a partial review: > > > > > 1. Even when a broker and a controller are co-located in the same JVM, > they must > > > have different node IDs > > > > Why? What problem are you trying to solve? > > > > Hi Jose, > > Thanks for the comments. > > We did talk about this a bit earlier in the thread. The controller is > always on its own port, even when it is running in the same JVM as a > broker. So it would not make sense to share the same ID here-- your > messages would not get through to the controller if you sent them to the > broker port instead. And clearly if the controller is on a separate host > from any broker, it can't share a broker id. > > > > > > 2. Node IDs must be set in the configuration file for brokers and > controllers. > > > > I understand that controller IDs must be static and in the > > configuration file to be able to generate consensus in KIP-595. Why > > are the broker nodes which are observers in KIP-595 cannot discover > > their ID on first boot and persist their ID for consistency in future > > restarts? > > > > This is discussed in the rejected alternatives section. Basically, node > ID auto-assignment is complicated and antiquated in the age of Kubernetes, > Chef, Puppet, Ansible, etc. > > > > > > 3. Controller processes will listen on a separate endpoint from brokers > > > > Why is this? Kafka supports multi endpoints. For example, one broker > > can have one endpoint for listening to connections by other brokers > > and another endpoint for connections from admin, producer and consumer > > clients. > > > > The reason for having separate ports is discussed in KIP-590. Basically, > it is so that control plane traffic can be isolated from data plane > traffic, as much as possible. This is the existing situation with > ZooKeeper. Since ZK is on a separate port, the client cannot disrupt the > cluster by flooding it with traffic (unless the admin has unwisely exposed > all internal ports, but this would cause bigger security issues). We want > to preserve this property. > > > > 4. In the case of controller RPCs like AlterIsr, the controller > handles this by not sending > > > back a response until the designated change has been persisted. > > > > Should we enumerate these RPCs? For example, we also have > > `ListPartitionReassignments` which is a read operation and goes > > directly to the controller. The naive solution would be to return the > > uncommitted state in the controller. > > > > Hmm. The KIP says that the "active controller must not make [...] future > state "visible" to the rest of the cluster until it has been made > persistent." So we don't return uncommitted state for read operations. > > > > > 5. > > This KIP mentions a topic named __kafka_metadata. KIP-595 and KIP-630 > > mention a partition named __cluster_metadata. We should reconcile this > > difference. > > > > Jason did write that the name of the controller topic is "not a formal > part of [the KIP-595] proposal." I think he wanted to avoid having the > discussion about the topic name in two different KIPs. :) > > Let's discuss the metadata topic name here in KIP-631, and update KIP-595 > as required once this one is accepted. > > best, > Colin > > > > > -- > > -Jose > > >
