Adar Dembo has posted comments on this change. ( )

Change subject: KUDU-2191 (10/n): Hive Metastore notification log event listener

Patch Set 7:

File src/kudu/master/catalog_manager.h:
PS7, Line 980: Protected by leader_lock_;
I think it makes more sense to protect with 'lock_', since this is cached 
in-memory state much like the info maps.

But see the comment I left around StoreLatestNotificationLogEventId; I think 
this will be cleanest if converted into a CowObject.
File src/kudu/master/
PS4, Line 2285: es while the HMS is
              :   // unavailable, because that would cause the catalogs t
> I've added more commentary, let me know if it's still confusing.
I was actually wrong in my statement anyway: we don't roll back HMS-initiated 
DROP TABLE either.

The only inconsistency between the two in behavior is that we'll send an ALTER 
to HMS even if it was originally initiated by HMS. You wrote that this helps 
keep the catalogs converged. Can you provide an example scenario? I'm asking 
because this seems a little fragile to me: I think it'd be easy for someone to 
accidentally (or intentionally) change HMS's no-op ALTER behavior to publish 
notification events anyway, at which point this ALTER would generate an 
infinite loop.
File src/kudu/master/
PS7, Line 2304: Rollback is skipped when the
              :   // alter is in response to a notification log event, since 
              :   // notification log events are automatically retried during 
the next polling
              :   // period.
This seems somewhat fragile since there's nothing actually enforcing that the 
polling task retries events whose Kudu-side changes failed. Meaning, someone 
could change the retry behavior tomorrow and then we'd have diverging catalogs.

Wouldn't it be simpler to do the rollback here anyway? SysCatalogTable::Write() 
failures are super rare, so it's not like it'd be a perf hit to rollback, right?
PS7, Line 2423:   // Update the cached HMS notification log event ID, if it 
I think it'd be cleaner if you routed all requests to update 
hms_notification_log_event_id_ through StoreLatestNotificationLogEventId(). For 
example, it'll let you centralize all of the locking in one place. To do that 
you'll need to parameterize the SysCatalogTable::Write().
PS7, Line 3935: Status CatalogManager::InitLatestNotificationLogEventId() {
DCHECK(hms_catalog_) here too, and in StoreLatest?
PS7, Line 3949:   // It wouldn't typically be correct to write to the field 
under only a read
              :   // lock, however this instance is safe because it's only 
called by the
              :   // HmsNotificationLogListenerTask singleton thread.
I'm not a fan of this shortcut. Why not use proper locking? Something like this:
1. Create lock::guard for some mutex (maybe a new one).
2. Create unique_lock lock_
3. Short-circuit success if hms_notification_log_event_id_ >= event_id
4. Release lock_
5. Perform the Write()
6. If Write() failed, return the error
7. Lock the unique_lock for lock_
8. Update hms_notification_log_event_id_
9. Return success

It's actually not much more work to store hms_notification_log_event_id_ as a 
CowObject and protect it via CowLock. It may even be simpler, since it'll hide 
the new mutex from the rest of CatalogManager by building it (well, an RWCLock) 
into the CowObject. Then this looks something like this:
1. CowLock hms_notification_log_event_id_ for write mode
2. Short-circuit success if hms_notification_log_event_id_ >= event_id
3. Update the value of hms_notification_log_event_id_ via mutable_data
4. Perform the Write()
5. If Write() failed, return the error
6. Commit the CowLock
7. Return success
File src/kudu/master/hms_notification_log_listener.h:
PS7, Line 86:   // Initialize the HMS notification log listener. The catalog 
manager must be
Nit: Initializes (to match the tense in Handle*Event). Please do the same for 
Init, Shutdown, and Poll too.
File src/kudu/master/
PS4, Line 35: #include "kudu/hms/hive_metastore_constants.h"
> I'm always in favor of shorter flag names over longer, however I feel it's
OK, that's a reasonable argument.
PS4, Line 116: ask::Poll(
> Isn't that just a special case of 'not leader'?
I suppose, but if you look at the various exit paths out of the 
ScopedLeaderSharedLock constructor, you'll see that not every error message is 
about not being the leader.

Anyway, it's just a VLOG, so I don't really care.
PS4, Line 291:   // Kudu catalog's metadata for the table. Checking the name in 
addition to the
             :   // ID prevents the HMS from altering a Kudu table when it has 
an outdated
             :   /
> No, the columns could have changed or some other piece of important metadat
Yeah but you're not including the new schema in 'req'. I assumed that was 
intentional; that this is for renames only. Was not the case?
File src/kudu/master/
PS7, Line 87:   CHECK_OK(ThreadJoiner(thread_.get()).Join());
Should probably null out thread_ when you're done so the CHECK on L85 will fire 
if someone calls Shutdown twice.
PS7, Line 201:   // The durable event ID gets updated every time we make a 
change in response
             :   // to a log notification, however not every log notification 
results in a
             :   // change (for instance, a notification pertaining to a 
Parquet table). To
             :   // avoid replaying these notifications we persist the latest 
             :   // notification log event ID after polling. This is best 
effort, since failing
             :   // to update the ID should only results in wasted work, not an 
             :   // catalog.
Hmm, upon second thought, updating the event ID with every reflected operation 
could lead to a lot of unnecessary deltas in the master tablet after having 
worked through a particularly large poll. Perhaps we shouldn't do that, and 
just update once here when the poll is finished?

The disadvantage to doing just one update is that if we lose leadership as 
we're processing HMS events, the new leader master will have to retry the 
entire HMS event set. But maybe that exact scenario is rare enough that the 
trade-off (reduced delta compaction in the master) is worth it. Certainly the 
code is less complex if we just update once.
PS7, Line 290: drop

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Hao Hao <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 11 Apr 2018 18:31:38 +0000
Gerrit-HasComments: Yes

Reply via email to