On Wed, Dec 2, 2020, at 14:07, Ron Dagostino wrote:
> Hi Colin. Thanks for the updates. It's now clear to me that brokers
> keep their broker epoch for the life of their JVM -- they register
> once, get their broker epoch in the response, and then never
> re-register again. Brokers may get fenced, but they keep the same
> broker epoch for the life of their JVM. The incarnation ID is also
> kept for the life of the JVM but is generated by the broker itself
> upon startup, and the combination of the two allows the Controller to
> act idempotently if any previously-sent registration response gets
> lost. Makes sense.
>
Thanks, Ron. That's a good summary.
> One thing I wonder about is if it might be helpful for the broker to
> send the Cluster ID as determined from its meta.properties file in its
> registration request. Does it even make sense for the broker to
> successfully register and enter the Fenced state if it has the wrong
> Cluster ID?
Yeah, that's a good idea. Let's have the broker pass its cluster ID in the registration RPC,
and then registration can fail if the broker is configured for the wrong cluster.
> The nextMetadatOffset value that the broker communicates
> in its registration request only has meaning within the correct
> cluster, so it feels to me that the Controller should have some way to
> perform this sanity check. There is currently (pre-KIP 500) a check
> in the broker to make sure its configured cluster ID matches the one
> stored in ZooKeeper, and we will have to perform this validation
> somewhere in the KIP-500 world. If the Controller doesn't do it
> within the registration request then the broker will have to make a
> metadata request to the Controller, retrieve the Cluster ID, and
> perform the check itself. It feels to me that it might be better for
> the Controller to just do it, and then the broker doesn't have to
> worry about it anymore once it successfully registers.
>
> I also have a question about the broker.id value and meta.properties.
> The KIP now says "In version 0 of meta.properties, there is a
> broker.id field. Version 1 does not have this field. It is no longer
> needed because we no longer support dynamic broker id assignment."
> But then there is an example version 1 meta.properties file that shows
> the broker.id value. I actually wonder if maybe the broker.id value
> would be good to keep in the version 1 meta.properties file because it
> currently (pre-KIP 500, version 0) acts as a sanity check to make sure
> the broker is using the correct log directory. Similarly with the
> controller.id value on controllers -- it would allow the same type of
> sanity check for quorum controllers.
>
That's a good point. I will add broker.id back, and also add controller.id as a possibility.
cheers,
Colin
>
> On Mon, Nov 30, 2020 at 7:41 PM Colin McCabe <cmccabe@apache.org> wrote:
> >
> > On Fri, Oct 23, 2020, at 16:10, Jun Rao wrote:
> > > Hi, Colin,
> > >
> > > Thanks for the reply. A few more comments.
> >
> > Hi Jun,
> >
> > Thanks again for the reply. Sorry for the long hiatus. I was on vacation for a
while.
> >
> > >
> > > 55. There is still text that favors new broker registration. "When a broker
> > > first starts up, when it is in the INITIAL state, it will always "win"
> > > broker ID conflicts. However, once it is granted a lease, it transitions
> > > out of the INITIAL state. Thereafter, it may lose subsequent conflicts if
> > > its broker epoch is stale. (See KIP-380 for some background on broker
> > > epoch.) The reason for favoring new processes is to accommodate the common
> > > case where a process is killed with kill -9 and then restarted. We want it
> > > to be able to reclaim its old ID quickly in this case."
> > >
> >
> > Thanks for the reminder. I have clarified the language here. Hopefully now it
is clear that we don't allow quick re-use of broker IDs.
> >
> > > 80.1 Sounds good. Could you document that listeners is a required config
> > > now? It would also be useful to annotate other required configs. For
> > > example, controller.connect should be required.
> > >
> >
> > I added a note specifying that these are required.
> >
> > > 80.2 Could you list all deprecated existing configs? Another one is
> > > control.plane.listener.name since the controller no longer sends
> > > LeaderAndIsr, UpdateMetadata and StopReplica requests.
> > >
> >
> > I added a section specifying some deprecated configs.
> >
> > > 83.1 It seems that the broker can transition from FENCED to RUNNING without
> > > registering for a new broker epoch. I am not sure how this works. Once the
> > > controller fences a broker, there is no need for the controller to keep the
> > > boker epoch around. So, if the fenced broker's heartbeat request with the
> > > existing broker epoch will be rejected, leading the broker back to the
> > > FENCED state again.
> > >
> >
> > The broker epoch refers to the broker registration. So we DO keep the broker epoch
around even while the broker is fenced.
> >
> > The broker epoch changes only when there is a new broker registration. Fencing
or unfencing the broker doesn't change the broker epoch.
> >
> > > 83.5 Good point on KIP-590. Then should we expose the controller for
> > > debugging purposes? If not, we should deprecate the controllerID field in
> > > MetadataResponse?
> > >
> >
> > I think it's OK to expose it for now, with the proviso that it won't be reachable
by clients.
> >
> > > 90. We rejected the shared ID with just one reason "This is not a good idea
> > > because NetworkClient assumes a single ID space. So if there is both a
> > > controller 1 and a broker 1, we don't have a way of picking the "right"
> > > one." This doesn't seem to be a strong reason. For example, we could
> > > address the NetworkClient issue with the node type as you pointed out or
> > > using the negative value of a broker ID as the controller ID.
> > >
> >
> > It would require a lot of code changes to support multiple types of node IDs. It's
not clear to me that the end result would be better -- I tend to think it would be worse,
since it would be more complex. In a similar vein, using negative numbers seems dangerous,
since we use negatives or -1 as "special values" in many places. For example, -1 often represents
"no such node."
> >
> > One important thing to keep in mind is that we want to be able to transition from
a broker and a controller being co-located to them no longer being co-located. This is much
easier to do when they have separate IDs.
> >
> > > 100. In KIP-589
> > > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-589+Add+API+to+update+Replica+state+in+Controller>,
> > > the broker reports all offline replicas due to a disk failure to the
> > > controller. It seems this information needs to be persisted to the
> > > metadata
> > > log. Do we have a corresponding record for that?
> > >
> >
> > Hmm, I have to look into this a little bit more. We may need a new record type.
> >
> > > 101. Currently, StopReplica request has 2 modes, without deletion and with
> > > deletion. The former is used for controlled shutdown and handling disk
> > > failure, and causes the follower to stop. The latter is for topic deletion
> > > and partition reassignment, and causes the replica to be deleted. Since we
> > > are deprecating StopReplica, could we document what triggers the stopping
> > > of a follower and the deleting of a replica now?
> > >
> >
> > RemoveTopic triggers deletion. In general the functionality of StopReplica is subsumed
by the metadata records.
> >
> > > 102. Should we include the metadata topic in the MetadataResponse? If so,
> > > when it will be included and what will the metadata response look like?
> > >
> >
> > No, it won't be included in the metadata response sent back from the brokers.
> >
> > > 103. "The active controller assigns the broker a new broker epoch, based on
> > > the latest committed offset in the log." This seems inaccurate since the
> > > latest committed offset doesn't always advance on every log append.
> > >
> >
> > Given that the new broker epoch won't be visible until the commit has happened,
I have changed this to "the next available offset in the log"
> >
> > > 104. REGISTERING(1) : It says "Otherwise, the broker moves into the FENCED
> > > state.". It seems this should be RUNNING?
> > >
> > > 105. RUNNING: Should we require the broker to catch up to the metadata log
> > > to get into this state?
> >
> > For 104 and 105, these sections have been reworked.
> >
> > best,
> > Colin
> >
> > >
> > > Thanks,
> > >
> > > Jun
> > >
> > >
> > >
> > > On Fri, Oct 23, 2020 at 1:20 PM Colin McCabe <cmccabe@apache.org> wrote:
> > >
> > > > On Wed, Oct 21, 2020, at 05:51, Tom Bentley wrote:
> > > > > Hi Colin,
> > > > >
> > > > > On Mon, Oct 19, 2020, at 08:59, Ron Dagostino wrote:
> > > > > > > Hi Colin. Thanks for the hard work on this KIP.
> > > > > > >
> > > > > > > I have some questions about what happens to a broker when
it becomes
> > > > > > > fenced (e.g. because it can't send a heartbeat request
to keep its
> > > > > > > lease). The KIP says "When a broker is fenced, it cannot
process any
> > > > > > > client requests. This prevents brokers which are not receiving
> > > > > > > metadata updates or that are not receiving and processing
them fast
> > > > > > > enough from causing issues to clients." And in the description
of the
> > > > > > > FENCED(4) state it likewise says "While in this state,
the broker
> > > > does
> > > > > > > not respond to client requests." It makes sense that a
fenced broker
> > > > > > > should not accept producer requests -- I assume any such
requests
> > > > > > > would result in NotLeaderOrFollowerException. But what
about KIP-392
> > > > > > > (fetch from follower) consumer requests? It is conceivable
that
> > > > these
> > > > > > > could continue. Related to that, would a fenced broker
continue to
> > > > > > > fetch data for partitions where it thinks it is a follower?
Even if
> > > > > > > it rejects consumer requests it might still continue to
fetch as a
> > > > > > > follower. Might it be helpful to clarify both decisions
here?
> > > > > >
> > > > > > Hi Ron,
> > > > > >
> > > > > > Good question. I think a fenced broker should continue to fetch
on
> > > > > > partitions it was already fetching before it was fenced, unless
it
> > > > hits a
> > > > > > problem. At that point it won't be able to continue, since
it doesn't
> > > > have
> > > > > > the new metadata. For example, it won't know about leadership
changes
> > > > in
> > > > > > the partitions it's fetching. The rationale for continuing
to fetch
> > > > is to
> > > > > > try to avoid disruptions as much as possible.
> > > > > >
> > > > > > I don't think fenced brokers should accept client requests.
The issue
> > > > is
> > > > > > that the fenced broker may or may not have any data it is supposed
to
> > > > > > have. It may or may not have applied any configuration changes,
etc.
> > > > that
> > > > > > it is supposed to have applied. So it could get pretty confusing,
and
> > > > also
> > > > > > potentially waste the client's time.
> > > > > >
> > > > > >
> > > > > When fenced, how would the broker reply to a client which did make
a
> > > > > request?
> > > > >
> > > >
> > > > Hi Tom,
> > > >
> > > > The broker will respond with a retryable error in that case. Once the
> > > > client has re-fetched its metadata, it will no longer see the fenced broker
> > > > as part of the cluster. I added a note to the KIP.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Tom
> > > > >
> > > >
> > >
>
|