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:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8313/7/src/kudu/master/catalog_manager.cc@3949
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 think it's pretty simple overall to do it "properly"

I don't really agree with this line of reasoning.  CowLock is a specialized 
locking primitive built for very specific use cases.  Meanwhile this is just a 
integer field that is already guaranteed to only be accessed by a single 
thread.  I'd buy the dedicated spinlock argument just in case someone else 
starts calling these APIs in the future, but I don't see why the CowLock would 
ever be justified.  Or it could just be made atomic.

> I'll also take this opportunity to reiterate that I think the implementation 
> would get a fair bit simpler if you did away with the 
> per-AlterTable/DropTable persistent event ID update and instead update it 
> just once at the end of a poll.

I've been thinking more about this since you posted your original comment.  The 
current implementation doesn't distinguish between events which originate in 
the Kudu Master and were mirrored to the HMS, and event which originate in the 
HMS.  Events in the first category should pretty much be ignored by the 
notification log listener; we don't need to take actions for events which we 
have, by definition, already acted on.  The second category is where we may 
need to make changes to the Kudu catalog.  In particular, there are two event 
types we need to handle: DROP TABLE and ALTER TABLE RENAME.

DROP TABLE is easy because the events are idempotent (since they are applied to 
a specific table ID).  We can apply these events over and over with no issue.

ALTER TABLE RENAME is more problematic because you can get A -> B -> A issues 
if you start applying events more than once, or out of order.  Right now we 
can't distinguish between events which originate in Kudu or the HMS, so we are 
susceptible to double-applying the Kudu events.  If we were to not persist the 
latest event ID along with the modifications, we would even be susceptible to 
double-applying HMS events in leadership failover scenarios.

Durably persisting the event ID along with the modification was an attempt to 
eliminate these double applications, however now I see that it can still happen 
(in the Kudu origin case).

So now I'm thinking we should prevent both cases by persisting the Kudu schema 
version with the HMS table entry.  Alterations which update the schema version 
should be considered as originating in Kudu, and not be double applied by the 
notification log listener.  Alterations which don't update the schema version 
should be considered as originating external to Kudu, and should be applied.  
However, the catalog manager should check that the schema version of the table 
matches, so that we don't apply an 'old' notification log entry.  This is 
actually 'safer' than the current implementation, which only checks that the 
table ID and table name match, but not the schema version (again, the issue is 
A -> B -> A rename loops).

So, to wrap this line of thought up and get back to your original suggestion, I 
think if we move to schema versions as the method of de-duplicating events, we 
could safely get away with only updating the hms_notification_log_event_id sys 
catalog field once per batch or poll period.



--
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: Thu, 12 Apr 2018 00:54:31 +0000
Gerrit-HasComments: Yes

Reply via email to