Add API GetPollerHistory #483

wxing1292 · 2017-12-26T07:53:05Z

DescribeTaskList return poller information, which contains the poller identity and last time this poller poll a given tasklist.

solve #467, #365, #144

yiminc-zz

At high level, one question is how do we add the Tasklist queue size and queue waiting time?

yiminc-zz · 2017-12-26T19:08:33Z

idl/github.com/uber/cadence/shared.thrift

+
+struct PollerInfo {
+  // ISO 8601 format
+  10: optional string timestamp


we are using UnixNano (i64) as timestamp in our APIs.

yiminc-zz · 2017-12-26T19:09:25Z

idl/github.com/uber/cadence/shared.thrift

+  10: optional list<PollerInfo> pollers
+}
+
+enum TaskListType {


Can we use TaskType here? We are going to use TaskListType for the type of task list like REGULAR, STICKY, and HOST_SPECIFIC.

at least right now, the task_list_type is already used in cassandra:

-- Stores activity or workflow tasks
CREATE TABLE tasks (
domain_id uuid,
task_list_name text,
task_list_type int, -- enum TaskListType {ActivityTask, DecisionTask}
type int, -- enum rowType {Task, TaskList}
task_id bigint, -- unique identifier for tasks, monotonically increasing
range_id bigint, -- Used to ensure that only one process can write to the table
task frozen,
task_list frozen<task_list>,
PRIMARY KEY ((domain_id, task_list_name, task_list_type), type, task_id)
) WITH COMPACTION = {
'class': 'org.apache.cassandra.db.compaction.LeveledCompactionStrategy'
};

yiminc-zz · 2017-12-26T19:11:41Z

idl/github.com/uber/cadence/cadence.thrift

+  /**
+  * GetPollerHistory returns pollers which poll from given tasklist in last few minutes.
+  **/
+  shared.GetPollerHistoryResponse GetPollerHistory(1: shared.GetPollerHistoryRequest request)


I thought we want expose something like DescTaskList. For now it only include the poller info, but soon we are going to expose the task list queue size and task list wait time.

i did just give some random name to the API and parameters, let us wait until @molteanu come back

yiminc-zz · 2017-12-26T19:42:16Z

service/matching/pollerHistory.go

+)
+
+type (
+	pollerIdentity struct {


the name is confusing. The identity is the identity supplied by worker which could be customized by user to any string. I think the idea here is to guarantee the uniqueness. Probably use some different name here to distinguish that would help understand the code.

well, there is a pollerID, which is actually a one time request ID; identity (which is specified in the API) is to some degree too generic......

let us wait for more comment about the names.

yiminc-zz

overall the poller info part looks good. I have concern about the implementation of the cache iterator, see inline comments.

yiminc-zz · 2018-01-11T19:49:00Z

common/cache/cache.go

+	Close()
+	// Entries returns a channel of MapEntry
+	// objects that can be used in a range loop
+	Entries() <-chan Entry


If user forget to call Close() it will leak resource. Why using goroutinue/channel instead of the HasNext() and Next() pattern?

ok but the iterator must have a close method,
this is because, if the release of the lock is handled inside the iterator, caller can choose not to iterator over all items in the lru cache.

type Iterator interface {
HasNext() bool
Next() Entry
Close()
}

yiminc-zz · 2018-01-11T19:51:40Z

common/cache/lru.go

+		stopCh: make(chan struct{}),
+	}
+
+	go func(iterator *iteratorImpl) {


I'm not sure we should use this approach for iterator. You are creating a goroutinue whenever iterator is needed. Goroutinue is cheap but it is not free. You also do context switch back and forth while iterate through the collection, which is much slower.

Why not just use the HasNext() and Next() pattern?

yiminc-zz · 2018-01-11T23:22:38Z

common/cache/lru_test.go

+				it := cache.Iterator()
+				defer it.Close()
+				for entry := range it.Entries() {
+					result = append(result, entry)


not clear what we are testing here. i don't see validation part.

this test test the concurrent execution of put / get / iteration. so as long as go test -race not reporting error, or there is no SIGSEGV, we are fine

yiminc-zz · 2018-01-11T23:37:02Z

common/convert.go

@@ -57,6 +61,12 @@ func BoolPtr(v bool) *bool {
 	return &v
 }

+// TimePtr makes a copy and returns the pointer to a string for time.Time, as ISO 8601 format.
+func TimePtr(v time.Time) *int64 {
+	time := v.UTC().UnixNano()


no need to call UTC().

yiminc-zz · 2018-01-12T00:09:47Z

service/matching/matchingEngine.go

+	for _, poller := range tlMgr.GetAllPollerInfo() {
+		pollers = append(pollers, &workflow.PollerInfo{
+			Identity:  common.StringPtr(poller.identity),
+			Timestamp: common.TimePtr(poller.timestamp),


this TimePtr is misleading, it is still int64 pointer.

cannot use Int64Ptr since golang does not support overloading, use TimeInt64Ptr

yiminc-zz · 2018-01-12T00:13:48Z

service/matching/pollerHistory.go

+	opts := &cache.Options{}
+	opts.InitialCapacity = pollerHistoryInitSize
+	opts.TTL = pollerHistoryTTL
+	opts.Pin = false


put the fields initialization in the field when create the struct.

yiminc-zz

I think there is a bug in the iterator.

yiminc-zz · 2018-01-12T04:13:11Z

common/cache/lru.go

+// Next return the next item
+func (it *iteratorImpl) Next() Entry {
+	if !it.HasNext() {
+		panic("LRU cache iterator Next called when there is no next item")


This is problematic because HasNext() checks TTL so it could return false even if you checked HasNext() before you call Next().

I think a better solution is to move the code in HasNext() to a new method, say prepareNext(), then in HasNext() you just return nextItem != nil, and Next() method simply return the value from nextItem, and before it returns, it move to next item and calls the prepareNext() for next calls. That way you only do validation once in prepareNext and you don't need an extra field timestamp in your iterator. (But you do need an initial call to prepareNext() when you create the iterator.)

yiminc-zz · 2018-01-12T04:14:46Z

common/convert.go

@@ -57,6 +61,12 @@ func BoolPtr(v bool) *bool {
 	return &v
 }

+// TimeInt64Ptr makes a copy and returns the pointer to a string for time.Time, as ISO 8601 format.
+func TimeInt64Ptr(v time.Time) *int64 {


remove this function and just use Int64Ptr(v.UnixNano()) instead.

i think this helper function is useful

We already have many places using Int64Ptr for timestamp. This new method only adds confusing to the codebase. The doc for this method is not correct. If you feel strong about this, you might want to send a separate diff for this refactor that updates all the other places as well.

yiminc-zz · 2018-01-12T18:30:07Z

common/cache/lru.go

+// Next return the next item
+func (it *iteratorImpl) Next() Entry {
+	if !it.HasNext() {
+		panic("LRU cache iterator Next called when there is no next item")


I think a better solution is to move the code in HasNext() to a new method, say prepareNext(), then in HasNext() you just return nextItem != nil, and Next() method simply return the value from nextItem, and before it returns, it move to next item and calls the prepareNext() for next calls. That way you only do validation once in prepareNext and you don't need an extra field timestamp in your iterator. (But you do need an initial call to prepareNext() when you create the iterator.)

yiminc-zz · 2018-01-12T18:35:48Z

common/convert.go

@@ -57,6 +61,12 @@ func BoolPtr(v bool) *bool {
 	return &v
 }

+// TimeInt64Ptr makes a copy and returns the pointer to a string for time.Time, as ISO 8601 format.
+func TimeInt64Ptr(v time.Time) *int64 {


We already have many places using Int64Ptr for timestamp. This new method only adds confusing to the codebase. The doc for this method is not correct. If you feel strong about this, you might want to send a separate diff for this refactor that updates all the other places as well.

yiminc-zz · 2018-01-12T18:46:40Z

common/cache/lru.go

 		existing := entry.value
 		if allowUpdate {
 			entry.value = value
 		}
 		if c.ttl != 0 {
-			entry.expiration = time.Now().Add(c.ttl)
+			entry.timestamp = time.Now()


Why are we change the expiration to timestamp? i think expiration is better because it is more clear what it means.

in terms of functionality, timestamp or expiration both works.
in terms of getting the last time a entry is refreshed, timestamp works, expiration do not. that is the reason

coveralls · 2018-01-12T22:36:26Z

Coverage decreased (-0.06%) to 66.618% when pulling 773e664 on poller into 65e165c on master.

make cache more generic

ae26fc6

wxing1292 self-assigned this Dec 26, 2017

wxing1292 requested review from samarabbas and yiminc-zz December 26, 2017 07:53

wxing1292 force-pushed the poller branch from ceed423 to 61ffbfa Compare December 26, 2017 16:55

add API GetPollerHistory

1958ca7

wxing1292 force-pushed the poller branch from 61ffbfa to 1958ca7 Compare December 26, 2017 18:24

yiminc-zz suggested changes Dec 26, 2017

View reviewed changes

wxing1292 requested a review from molteanu December 26, 2017 20:12

wxing1292 force-pushed the poller branch from ecb0cd1 to 17709c6 Compare January 5, 2018 18:58

rename GetPollerHistory to DescribeTaskList

86d437a

wxing1292 force-pushed the poller branch from 17709c6 to 86d437a Compare January 5, 2018 20:50

yiminc-zz suggested changes Jan 12, 2018

View reviewed changes

wxing1292 force-pushed the poller branch from 0f213d1 to ed1b37c Compare January 12, 2018 02:23

address comments

5ced7dd

wxing1292 force-pushed the poller branch from ed1b37c to 5ced7dd Compare January 12, 2018 02:27

yiminc-zz suggested changes Jan 12, 2018

View reviewed changes

address comments

31539e8

yiminc-zz suggested changes Jan 12, 2018

View reviewed changes

Wenquan Xing added 2 commits January 12, 2018 12:12

address comments

c681220

address more comments

048a17b

yiminc-zz approved these changes Jan 12, 2018

View reviewed changes

Wenquan Xing and others added 2 commits January 12, 2018 14:11

more comments

f2cdce2

Merge branch 'master' into poller

773e664

wxing1292 merged commit 5121974 into master Jan 12, 2018

wxing1292 deleted the poller branch January 12, 2018 23:07

meiliang86 pushed a commit to meiliang86/cadence that referenced this pull request Jul 17, 2020

Remove executable permission from JSON files (cadence-workflow#483)

48a8724

meiliang86 pushed a commit that referenced this pull request Jul 21, 2020

Remove executable permission from JSON files (#483)

078cdb4

yux0 pushed a commit to yux0/cadence that referenced this pull request May 4, 2021

Remove executable permission from JSON files (cadence-workflow#483)

9b595e2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API GetPollerHistory #483

Add API GetPollerHistory #483

wxing1292 commented Dec 26, 2017 •

edited

Loading

yiminc-zz left a comment

yiminc-zz Dec 26, 2017

wxing1292 Jan 5, 2018

yiminc-zz Dec 26, 2017

wxing1292 Jan 5, 2018

yiminc-zz Dec 26, 2017

wxing1292 Dec 26, 2017

yiminc-zz Dec 26, 2017

wxing1292 Dec 26, 2017

yiminc-zz left a comment

yiminc-zz Jan 11, 2018

wxing1292 Jan 12, 2018 •

edited

Loading

yiminc-zz Jan 11, 2018

yiminc-zz Jan 11, 2018

wxing1292 Jan 12, 2018

yiminc-zz Jan 11, 2018

wxing1292 Jan 12, 2018

yiminc-zz Jan 12, 2018

wxing1292 Jan 12, 2018

yiminc-zz Jan 12, 2018

wxing1292 Jan 12, 2018

yiminc-zz left a comment

yiminc-zz Jan 12, 2018

wxing1292 Jan 12, 2018

yiminc-zz Jan 12, 2018

yiminc-zz Jan 12, 2018

wxing1292 Jan 12, 2018

yiminc-zz Jan 12, 2018

wxing1292 Jan 12, 2018

yiminc-zz Jan 12, 2018

yiminc-zz Jan 12, 2018

yiminc-zz Jan 12, 2018

wxing1292 Jan 12, 2018

coveralls commented Jan 12, 2018

Add API GetPollerHistory #483

Add API GetPollerHistory #483

Conversation

wxing1292 commented Dec 26, 2017 • edited Loading

yiminc-zz left a 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

yiminc-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxing1292 Jan 12, 2018 • 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

yiminc-zz left a 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 Jan 12, 2018

wxing1292 commented Dec 26, 2017 •

edited

Loading

wxing1292 Jan 12, 2018 •

edited

Loading