RetMgr: Handle cleanup of consumer-groups and consumer-group extents #310

kirg · 2017-10-10T20:11:12Z

This diff also adds a new API to list consumer-groups for a given destination-uuid -- the difference between this and the existing ListConsumerGroups API is that this one would look at the 'consumer_groups' table (instead of 'consumer_groups_by_name', that ListConsumerGroups look at).

Retention-manager uses this new API to query consumer-groups in 'deleting' state and takes appropriate action to 'delete' them.

datoug · 2017-10-12T17:51:41Z

clients/metadata/metadata_cassandra.go

@@ -1189,9 +1189,26 @@ const (
 		columnZoneConfigs + `: ?, ` +
 		columnOptions + `: ? }`

+	cqlConsumerGroupType = columnConsumerGroup + `.` + columnUUID + "," +


Nit: sqlConsumerGroupType (to be consistent)

true .. :-)

datoug · 2017-10-12T18:08:34Z

services/retentionmgr/metadataDep.go

@@ -60,7 +60,7 @@ func (t *metadataDepImpl) GetDestinations() (destinations []*destinationInfo) {
 	i := 0
 	for {

-		log.Debug("GetDestinations: ListDestinationsByUUID on metadata")
+		log.Info("GetDestinations: ListDestinationsByUUID on metadata")


I think these will cause too many logs

not sure now .. why i decided to enable these. reverting it. :-)

datoug · 2017-10-12T18:10:04Z

services/retentionmgr/metadataDep.go

+		if e != nil {
+			err = e
+			log.WithField(common.TagErr, err).Error("GetExtentsForConsumerGroup: ReadConsumerGroupExtentsLite failed")
+			break


return here?

i do a 'break' so it will print the "done" log below ..

datoug · 2017-10-12T18:12:58Z

services/retentionmgr/metadataDep.go

+func (t *metadataDepImpl) DeleteConsumerGroup(destID destinationID, cgID consumerGroupID) error {
+
+	req := metadata.NewDeleteConsumerGroupUUIDRequest()
+	req.UUID = common.StringPtr(string(destID))


uuid should be cg uuid right?

yes; fixed.

datoug · 2017-10-12T18:13:38Z

clients/metadata/metadata_cassandra.go

 	sqlInsertCGByUUID = `INSERT INTO ` + tableConsumerGroups +
 		`(` +
 		columnUUID + `, ` +
+		columnDestinationUUID + `, ` +
 		columnIsMultiZone + `, ` +
 		columnConsumerGroup +
 		`) VALUES (?, ?,` + sqlCGValue + `)`


You need one more '?' here

some test should be able to catch this.

datoug · 2017-10-12T18:22:06Z

services/retentionmgr/retention.go

@@ -342,10 +435,9 @@ func (t *RetentionManager) runRetention(jobsC chan<- *retentionJob) bool {
 			totalJobs++
 		}

-		if allExtentsDeleted && dest.status == shared.DestinationStatus_DELETING {
+		if dest.status == shared.DestinationStatus_DELETING && numCGs == 0 && numExtents == 0 {


I don't think you can rely on 'numCGs == 0' since the error from the below call was swallowed.
cgs := t.metadata.GetConsumerGroups(dest.id)

I still don't see this get fixed.

datoug · 2017-10-12T18:23:25Z

clients/metadata/metadata_cassandra.go

@@ -2003,6 +1987,73 @@ func (s *CassandraMetadataService) ListConsumerGroups(ctx thrift.Context, reques
 	return result, nil
 }

+// ListConsumerGroupsUUID returns all ConsumerGroups matching the given destination-uuid.
+func (s *CassandraMetadataService) ListConsumerGroupsUUID(ctx thrift.Context, request *shared.ListConsumerGroupsUUIDRequest) (*shared.ListConsumerGroupsUUIDResult_, error) {


can you add some UT for the new API?

i have now.

datoug · 2017-10-12T18:24:07Z

services/retentionmgr/retention.go

@@ -312,26 +314,117 @@ func (t *RetentionManager) runRetention(jobsC chan<- *retentionJob) bool {
 			continue
 		}

+		log := t.logger.WithField(common.TagDst, dest.id)


Is it possible to UT the new behavior

it can be .. though it will be quite some work; will give it a shot.

as discussed offline, i am going to take this up separately.

coveralls · 2017-10-17T06:56:32Z

Coverage increased (+0.5%) to 67.431% when pulling 262378c on cg-dest-uuid into 985ade9 on master.

coveralls · 2017-10-18T18:04:07Z

Coverage increased (+0.2%) to 67.541% when pulling 4c71fb1 on cg-dest-uuid into fc00e42 on master.

datoug · 2017-10-19T18:23:25Z

clients/metadata/metadata_cassandra_test.go

-	inputs = append(inputs, &shared.ListConsumerGroupRequest{
+	}).Equals(cgSet), "ListConsumerGroups did not return all CGs")
+
+	assert.True(listCGs(&shared.ListConsumerGroupRequest{


Is this section duplicated from previous one?

yes. removed.

coveralls · 2017-10-20T18:31:48Z

Coverage increased (+0.07%) to 67.571% when pulling 1abe1d8 on cg-dest-uuid into 3ef749a on master.

* Revert "Slowdown redeliveries, add timeout metrics (#290)" This reverts commit 464433e. * cmq - Cherami Metadata Query-r * ttt * show remote extents * remove binary * fix rate computation * flag DLQ extents * monitor -> watch * Revert "Slowdown redeliveries, add timeout metrics (#290)" (#316) This reverts commit 464433e. * sort by cgx-status updated time * show milliseconds since update * mark cgx for consumed extent as consumed * ~ * ~ * RetMgr: Handle cleanup of consumer-groups and consumer-group extents (#310) * handle consumer-group deletion * fix mocks, tests * debug log * more log * log * metadata logs * add destination_uuid column to consumer_groups table * ListConsumerGroupsUUID * Make RetMgr use ListConsumerGroupsUUID * update meta-metrics * thrift * thrift * update the new dest-uuid column in create-, update- and delete- cg paths * fix create-cg cql * fix mocks * ListConsumerGroupsUUID test * CR feedback * return and catch errors from metadata in ret-mgr * incorporate cr feedback * ~ * ensure consumer-groups are deleted before deleting destination * fix build due to gocql API change * cmq - Cherami Metadata Query-r * ttt * show remote extents * remove binary * fix rate computation * flag DLQ extents * monitor -> watch * sort by cgx-status updated time * show milliseconds since update * mark cgx for consumed extent as consumed * ~ * ~ * ... * cg deleting * fix build * yaml config support * cmq zone * output type support * json map output * fix lint * fix lint * fix build * fix lint

kirg requested review from kobeyang and datoug October 10, 2017 20:11

datoug reviewed Oct 12, 2017

View reviewed changes

kirg mentioned this pull request Oct 19, 2017

RetMgr: Handle cleanup of consumer-groups and consumer-group extents #304

Closed

kiranrg added 18 commits October 19, 2017 10:55

handle consumer-group deletion

f833838

fix mocks, tests

7f0038b

debug log

e8423e1

more log

14c0877

log

2f9b6ff

metadata logs

aefff4f

add destination_uuid column to consumer_groups table

885b290

ListConsumerGroupsUUID

a6ae039

Make RetMgr use ListConsumerGroupsUUID

b90267b

update meta-metrics

e125318

thrift

df02f11

thrift

9ee4008

update the new dest-uuid column in create-, update- and delete- cg paths

ed2765f

fix create-cg cql

6325258

fix mocks

e3d590f

ListConsumerGroupsUUID test

c674b61

CR feedback

3979198

return and catch errors from metadata in ret-mgr

33fc0c8

kirg force-pushed the cg-dest-uuid branch from 0c2e941 to 33fc0c8 Compare October 19, 2017 17:56

datoug reviewed Oct 19, 2017

View reviewed changes

kiranrg added 3 commits October 19, 2017 13:50

incorporate cr feedback

d36c008

~

6c70a9c

ensure consumer-groups are deleted before deleting destination

f63bfdf

kirg changed the title ~~Add ListConsumerGroupsUUID metadata API~~ RetMgr: Handle cleanup of consumer-groups and consumer-group extents Oct 20, 2017

fix build due to gocql API change

1abe1d8

datoug approved these changes Oct 20, 2017

View reviewed changes

kirg merged commit 5652d7a into master Oct 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RetMgr: Handle cleanup of consumer-groups and consumer-group extents #310

RetMgr: Handle cleanup of consumer-groups and consumer-group extents #310

kirg commented Oct 10, 2017 •

edited

Loading

datoug Oct 12, 2017

kirg Oct 18, 2017

datoug Oct 12, 2017

kirg Oct 18, 2017

datoug Oct 12, 2017

kirg Oct 18, 2017

datoug Oct 12, 2017

kirg Oct 18, 2017

datoug Oct 12, 2017

datoug Oct 12, 2017

datoug Oct 12, 2017

datoug Oct 20, 2017

datoug Oct 12, 2017

kirg Oct 18, 2017

datoug Oct 12, 2017

kirg Oct 18, 2017

kirg Oct 20, 2017

coveralls commented Oct 17, 2017 •

edited

Loading

coveralls commented Oct 18, 2017 •

edited

Loading

datoug Oct 19, 2017

kirg Oct 19, 2017

coveralls commented Oct 20, 2017 •

edited

Loading

RetMgr: Handle cleanup of consumer-groups and consumer-group extents #310

RetMgr: Handle cleanup of consumer-groups and consumer-group extents #310

Conversation

kirg commented Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 17, 2017 • edited Loading

coveralls commented Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 20, 2017 • edited Loading

kirg commented Oct 10, 2017 •

edited

Loading

coveralls commented Oct 17, 2017 •

edited

Loading

coveralls commented Oct 18, 2017 •

edited

Loading

coveralls commented Oct 20, 2017 •

edited

Loading