Implement describeMutableState #805

longquanzheng · 2018-05-31T19:06:53Z

Initial PR, I will implement unit test for HistoryCache if there is no major issue in the code change.

arthurgan · 2018-05-31T23:17:58Z

client/history/client.go

-	thriftCache     map[string]historyserviceclient.Interface
-	rpcFactory      common.RPCFactory
+	// TODO: consider refactor InquireCache into a separate struct
+	InquireCacheLock sync.RWMutex


Are these intentionally Upper case?

arthurgan · 2018-05-31T23:20:28Z

client/history/client.go

-	c.thriftCacheLock.RLock()
-	client, ok := c.thriftCache[hostPort]
-	c.thriftCacheLock.RUnlock()
+	c.InquireCacheLock.RLock()


If you just do:
c.InquireCacheLock.RLock()
defer c.InquireCacheLock.Unlock()
here, you won't have to acquire the lock again and the lock is cleaner?

As above, I mistakenly changed the naming and this is not part of my scope. But we can open another issue to optimize the locking mechanism.
One reason I can see that why they did this, is they want to release the lock earlier? But I can see that they did that on purpose to improve the performance, as the trade off, it will sacrify some readability

yiminc-zz · 2018-05-31T23:18:07Z

client/history/client.go

-	thriftCache     map[string]historyserviceclient.Interface
-	rpcFactory      common.RPCFactory
+	// TODO: consider refactor InquireCache into a separate struct
+	InquireCacheLock sync.RWMutex


why this change?

This is a mistake when I refactor the naming. I will change it back. Thanks for reminding.

yiminc-zz · 2018-05-31T23:21:36Z

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

  10: optional string shardId
  20: optional string historyAddr
+  30: optional string otherInfo


what is this?

@wxing1292 said he need more information in mutableStateBuilder like update/deleteActivityInfos, making sure that they are empty.

but not called otherInfo, as a string, the idea is to dump all things from mutable state (cache version and persistence version).
the original type is preferable

moreover, shardId is a int64, i believe.

Talked offline, I will get rid of otherInfo

yiminc-zz · 2018-05-31T23:26:56Z

service/history/historyCache.go

+	contextFromCache, cacheHit := c.Get(key).(*workflowExecutionContext)
+	releaseFunc := func(error) {}
+	// If cache hit, we need to lock the cache to prevent race condition
+	if cacheHit {


this feels complicated, it would be simpler if we just return from whatever in the cache, and provide another API to force unload the cache, which is also needed as an admin command.

This s about requirement of the task. I will talk to you in uChat

yiminc-zz · 2018-05-31T23:28:19Z

service/history/historyEngine.go

+		retResp.MutableStateInCache, retError = e.toMutableStateJSON(msb)
+		otherInfo := ""
+		if len(msb.updateActivityInfos) > 0 {
+			otherInfo += "updateActivityInfos is not empty;"


why do we need this otherInfo? The toMutableStateJSON should already include everything, right?

Like above. there are still someother information like updateActivityInfos. @wxing1292 you guys can discuss if they are really needed. I don't have too much context of how we are going to use it.

well, i still prefer dumping everything as original type, not string

yiminc-zz · 2018-05-31T23:31:04Z

service/history/mutableStateBuilder.go

@@ -169,6 +169,22 @@ func newMutableStateBuilderWithReplicationState(config *Config, logger bark.Logg
 	return s
 }

+func (e *mutableStateBuilder) Unload() *persistence.WorkflowMutableState {


The function name is unclear. Maybe name it as CopyTo...

I felt so too. did that because we already have

func (e *mutableStateBuilder) Load(state *persistence.WorkflowMutableState)"

doing the opposite.

CopyTo might not be a better name because mutableStateBuilder and mutabeState struct are not exactly the same.
To be more clear, we probably can call it loadFromMutableState and unloadToMutableState.

Unload give false impression that you are resetting the mutable state.

Make sense. How about rename it to "copyTo"?

arthurgan

Approved because all my comments are addressed.

However, maybe wait for another approval(like from Yimin) before merging.

wxing1292 · 2018-06-03T07:04:50Z

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

  **/
-  InquiryWorkflowExecutionResponse InquiryWorkflowExecution(1: shared.DescribeWorkflowExecutionRequest inquiryRequest)
+  InquireWorkflowExecutionResponse InquireWorkflowExecution(1: InquireWorkflowExecutionRequest inquireRequest)


let us use simple words here, try describe or get.

+1 for Describe

OK, I will rename them to DescribeXX

yiminc-zz

Looks good. With 2 minor naming suggestion.

yiminc-zz · 2018-06-04T20:51:25Z

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

  **/
-  InquiryWorkflowExecutionResponse InquiryWorkflowExecution(1: shared.DescribeWorkflowExecutionRequest inquiryRequest)
+  InquireWorkflowExecutionResponse InquireWorkflowExecution(1: InquireWorkflowExecutionRequest inquireRequest)


+1 for Describe

yiminc-zz · 2018-06-04T20:53:49Z

service/history/mutableStateBuilder.go

@@ -169,6 +169,22 @@ func newMutableStateBuilderWithReplicationState(config *Config, logger bark.Logg
 	return s
 }

+func (e *mutableStateBuilder) Unload() *persistence.WorkflowMutableState {


Unload give false impression that you are resetting the mutable state.

longquanzheng requested review from venkat1109, wxing1292, yiminc-zz and vancexu May 31, 2018 19:06

longquanzheng mentioned this pull request May 31, 2018

Admin CLI for Cadence servicing operations #677

Closed

arthurgan reviewed May 31, 2018

View reviewed changes

yiminc-zz reviewed May 31, 2018

View reviewed changes

arthurgan approved these changes Jun 1, 2018

View reviewed changes

wxing1292 reviewed Jun 3, 2018

View reviewed changes

wxing1292 approved these changes Jun 4, 2018

View reviewed changes

yiminc-zz approved these changes Jun 4, 2018

View reviewed changes

longquanzheng added 7 commits June 4, 2018 18:55

Implement inquireMutableState

7864852

add copy right

67d6d8d

fix unit test

364aa5f

fix renaming mistake

37bd044

get rid of otherInfo

413cbfa

rename unload to copyTo

a9a9ef1

rename inquire to describe

d4c89a6

longquanzheng force-pushed the admin-dev-2 branch from 7c3c6ae to d4c89a6 Compare June 5, 2018 01:55

longquanzheng merged commit 772d653 into cadence-workflow:master Jun 5, 2018

longquanzheng changed the title ~~Implement inquireMutableState~~ Implement describeMutableState Jun 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement describeMutableState #805

Implement describeMutableState #805

longquanzheng commented May 31, 2018

arthurgan May 31, 2018

arthurgan May 31, 2018

longquanzheng Jun 1, 2018

arthurgan Jun 1, 2018

yiminc-zz May 31, 2018

longquanzheng Jun 1, 2018

yiminc-zz May 31, 2018

longquanzheng Jun 1, 2018

wxing1292 Jun 3, 2018 •

edited

Loading

longquanzheng Jun 4, 2018

yiminc-zz May 31, 2018

longquanzheng Jun 1, 2018

yiminc-zz May 31, 2018

longquanzheng Jun 1, 2018

wxing1292 Jun 3, 2018

yiminc-zz May 31, 2018

longquanzheng Jun 1, 2018 •

edited

Loading

yiminc-zz Jun 4, 2018

longquanzheng Jun 4, 2018

arthurgan left a comment

wxing1292 Jun 3, 2018

yiminc-zz Jun 4, 2018

longquanzheng Jun 4, 2018

yiminc-zz left a comment

yiminc-zz Jun 4, 2018

yiminc-zz Jun 4, 2018

Implement describeMutableState #805

Implement describeMutableState #805

Conversation

longquanzheng commented May 31, 2018

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

wxing1292 Jun 3, 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

longquanzheng Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurgan 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

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

wxing1292 Jun 3, 2018 •

edited

Loading

longquanzheng Jun 1, 2018 •

edited

Loading