Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

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


Patch Set 7:

(23 comments)

I still need to address Adar's request to doc and test the changes in 
HmsCatalog, but this is already getting to be a big diff and I need to switch 
over to another high priority task, so pushing as is.

http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:

PS4:
> Revert the changes here?
Done


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/integration-tests/master_hms-itest.cc@104
PS4, Line 104:                              
"hive_metastore_notification_log_poll_period_seconds", "0");
> Nit: could you refer to the gflag with underscores? Makes it easier to find
Done


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/integration-tests/master_hms-itest.cc@105
PS4, Line 105:   }
> Nit: "0" isn't good enough? :)
Hah, I think my brain was on type-system autopilot there.


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/catalog_manager.cc@708
PS4, Line 708:   std::lock_guard<LockType> l(lock_);
             :   background_tasks_.reset(new CatalogManagerBgTasks(this));
             :   RETURN_NOT_OK_PREPEND(background_tasks_->Init(),
             :                         "Failed to initialize catalog manager 
background tasks");
             :
> Why not do this on L701 right after starting the HmsCatalog? Could avoid th
Done


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/catalog_manager.cc@2273
PS4, Line 2273:     // If some tablet schemas need to be updated or there are 
any new tablets,
> So why is this no longer conditioned on has_metadata_changes? We want ADD/D
Not so much for the partitions cases (although it doesn't hurt), but we want 
all alters to issue an HmsCatalog alter, since it helps keep the catalogs 
converged.  HmsCatalog will automatically short circuit if detects no changes 
need to be made.  Additionally, we need to persist the hms_notification_log_id 
if it's not none_t.


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/catalog_manager.cc@2285
PS4, Line 2285: es while the HMS is
              :   // unavailable, because that would cause the catalogs t
> It isn't obvious why we don't roll back an HMS-initiated ALTER TABLE but we
I've added more commentary, let me know if it's still confusing.


http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc
File src/kudu/master/hms_catalog.cc:

http://gerrit.cloudera.org:8080/#/c/8313/3/src/kudu/master/hms_catalog.cc@270
PS3, Line 270:
> I commented on this in the newer iteration of the patch: I still think medi
Ack


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.h
File src/kudu/master/hms_notification_log_listener.h:

http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.h@41
PS4, Line 41: //
> Doc the class and functions.
Done


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.h@66
PS4, Line 66: //
            : // - DROP TABLE
            : //    The notification log
> Given the way you're using the lock, the cv, and the atomic bool, maybe a C
Great idea.


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@35
PS4, Line 35: #include "kudu/hms/hive_metastore_constants.h"
> These gflags are rather long; perhaps we can shorten "hive_metastore..." to
I'm always in favor of shorter flag names over longer, however I feel it's 
important to retain the full name here in order to remain consistent with the 
flags defined in hms_catalog.cc, and in particular the 'hive_metastore_uris' 
flag, which is the master switch of enabling HMS integration.  I don't want to 
shorten 'hive_metastore_uris' because the current name matches the 
corresponding Hive client config option in hive-site.xml.

FWIW I only expect these two flags to be tweaked in exceptional circumstances.


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@61
PS4, Line 61:
> Unused? Was this supposed to be referenced on L44?
woops, this was leftover from before I flag-ified it.


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@116
PS4, Line 116: ask::Poll(
> That's a little broad; the catalog manager could be in the process of start
Isn't that just a special case of 'not leader'?


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@142
PS4, Line 142:       // If we can't connect to the HMS then we return early, 
which will result
> Hmm, but all we do is return, like some of the other code paths out. Was th
Woops, yep this is a stale comment.


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@169
PS4, Line 169:         if (l.has_term_changed()) {
> Should comment that even though we may have processed some events, there's
Done


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@181
PS4, Line 181:           // This happens for every rename/drop table event that 
originates
             :           // in Kudu, so it's too noisy to log
> So suppose we're in release mode and we end up here, meaning we tried to re
Done


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@191
PS4, Line 191:
             :
             :       processed_event_id = event.eventId;
> Why not wait until all batches are done and then persist the change once?
Done


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@211
PS4, Line 211:   }
> Nit: std::lock_guard
Done


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@222
PS4, Line 222:
> Just curious: is there anything here that needs to be redacted? Or in any o
I don't think so.  We don't consider table metadata to be sensitive.


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@251
PS4, Line 251: vent: $0", EventDebugString(event)));
> You introduced a new constant for this, no? Below too.
Done


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@291
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
             :   /
> If the two are the same, why bother doing the AlterTable at all? Isn't it j
No, the columns could have changed or some other piece of important metadata.


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/hms_notification_log_listener.cc@349
PS4, Line 349:                               EventDebugString(event));
             :   }
             :
             :   LOG(INFO) << "Dropping table " << table_name << " [id=" << 
*table_id << "] "
             :             << "in response to event: " << 
EventDebugString(event);
             :
> This is true for AlterTable as well, right?
Done


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:

http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/sys_catalog.cc@816
PS4, Line 816: void 
SysCatalogTable::ReqSetNotificationLogEventId(WriteRequestPB* req, int64_t 
event_id) {
> While clever, I think it would be clearer to explicitly define a super simp
Done


http://gerrit.cloudera.org:8080/#/c/8313/4/src/kudu/master/sys_catalog.cc@824
PS4, Line 824:   CHECK_OK(row.SetInt8(kSysCatalogTableColType, 
HMS_NOTIFICATION_LOG));
> Maybe define a magic value for this up front?
Done



--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Hao Hao <hao....@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 11 Apr 2018 16:37:29 +0000
Gerrit-HasComments: Yes

Reply via email to