Add RPC #97

zklgame · 2023-11-14T13:24:48Z

Why make this pull request?

Add RPC.

Java SDK Integ Test: xcherryio/sdk-java#13

What has changed

[Summarize what components of the repo is updated]

[Link to xdb-apis/xdb-golang-sdk PRs if it's on top of any API changes]

API change link: ...
Golang SDK change link: ...
Server Component 1: ...
Server Component 2: ...

How to test this pull request?

[If writing Integration test in Golang SDK repo, please provide link to the pull request of Golang SDK Repo]

[It's recommended to write integration test in Golang SDK repo, and enabled in this server repo first,
without enabling in the SDK repo. After this PR is merged, enable and merge the integration test in the SDK repo]

[Alternatively if Java/other SDK repo is preferred, then just test locally against server PR.
After the server PR is merged, merge the integration test in the SDK repo]

Checklist before merge

[ ] If applicable, merge the xdb-apis/xdb-golang-sdk PRs to main branch
[ ] If applicable, merge the xdb-apis/xdb-apis PRs to main branch
[ ] Update go.mod to use the commitID of the main branches for xdb-apis/xdb-golang-sdk

longquanzheng · 2023-11-15T19:59:41Z

config/config.go

@@ -54,6 +54,8 @@ type (
 		InternalHttpServer HttpServerConfig `yaml:"internalHttpServer"`
 		// ClientAddress is the address for API service to call AsyncService's internal API
 		ClientAddress string `yaml:"clientAddress"`
+		// Rpc is the config for rpc calls
+		Rpc RpcConfig `yaml:"rpc"`


I think RPC should live under API service

True, modified.

longquanzheng · 2023-11-15T20:01:11Z

service/api/service_impl.go

+		return nil, s.handleUnknownError(err)
+	}
+
+	if latestPrcExe.NotExists || latestPrcExe.Status != data_models.ProcessExecutionStatusRunning {


in IWF, a read-only RPC is allowed for a closed workflow execution. Do you think we can still support it?

Sounds good. I moved the second checking condition later to skip writing if the process is not running after receiving the RPC response.

longquanzheng · 2023-11-15T20:03:40Z

service/api/service_impl.go

+		s.logger.Warn("failed to update global attributes")
+		return nil, NewErrorWithStatus(
+			http.StatusFailedDependency,
+			"Failed to write global attributes, please check the error message for details: "+updateResp.UpdatingGlobalAttributesError.Error())


Error handling is critical for building reliable applications. I think we need to return the error code to applications as mentioned in xcherryio/apis#24 (comment)

http.StatusFailedDependency means 424, this is the code.

I meant the error code from the database driver. The global attribute is mapped to database, the error code from database is like this: https://github.com/xdblab/xdb/blob/main/extensions/postgres/error_checker.go#L33

Why we need this error code --
the user code will make different decisions on this. If column value is conflict, or database schema is invalid, they should return different error code and different UI message in the application.

You wrote it:
https://github.com/xdblab/xdb/blob/689585f839c05e29ffc0b3864e95fd002bd34b06/persistence/sql/complete_execute.go#L49

This is in async state, we don’t have a good way to return error. We can discuss this in zoom

And I also need to improve the error code in start process too to return error code

I meant the error code from the database driver.

The error code from the database driver should be returned by the updateGlobalAttributesIfNeeded method first. And here in the implementation for RPC, the UpdatingGlobalAttributesError is assigned with whatever returned from updateGlobalAttributesIfNeeded, and it's returned to SDK with NewErrorWithStatus.

So if you wanna imporve on the error message. you can just update updateGlobalAttributesIfNeeded itself in another PR. Currently, I will just return whatever the err is from the updateGlobalAttributesIfNeeded to SDK.

longquanzheng · 2023-11-15T20:06:22Z

persistence/sql/update_process_execution_from_rpc.go

+
+	// if getting next states and there are running state(s), return error
+	if len(request.StateDecision.GetNextStates()) > 0 && len(sequenceMaps.PendingExecutionMap) > 0 {
+		return nil, fmt.Errorf("cannot start new states when there are running state(s)")


I don't think this is a valid assumption. There are use cases that need to start new thread while some states are running

Removed it.

longquanzheng · 2023-11-15T20:08:10Z

service/api/service_impl.go

+func (s serviceImpl) Rpc(
+	ctx context.Context, request xdbapi.ProcessExecutionRpcRequest,
+) (response *xdbapi.ProcessExecutionRpcResponse, retErr *ErrorWithStatus) {
+	latestPrcExe, err := s.store.GetLatestProcessExecution(ctx, data_models.GetLatestProcessExecutionRequest{


I don't think it's safe outside a transaction. The process execution could get terminated or even deleted right after this query.

s.store.GetLatestProcessExecution relies on session.SelectLatestProcessExecution, that have been used several times in the server. Do you mean we modify the session.SelectLatestProcessExecution to tx.SelectLatestProcessExecution for all the usages? If not, why we only need this ONE place to be in the transaction?

And you cannot put it into transaction because later there will be a API call to the worker. This is similar to how the waitUntil/Execute API works.

Yeah just found two other places in doStopProcessTx and doPublishToLocalQueueTx.

I missed these two in the previous PRs. Sorry. It will have racing conditions in these two APIs as well.

Then we need to re-design ALL the transactions that query the table xdb_sys_latest_process_executions to lock the row in xdb_sys_latest_process_executions as the first step in each transation.

The GetLatestProcessExecution before worke RPC call will still be in the session. Once getting the worker RPC response, we will directly use the tx.SelectProcessExecutionForUpdate to lock.

And two other places in doStopProcessTx and doPublishToLocalQueueTx can use sessions to SelectLatestProcessExecution, I don't think there will be race condition. Can you just provide a detailed example to prove the opposite side?

codecov · 2023-11-16T00:46:58Z

Codecov Report

Attention: 257 lines in your changes are missing coverage. Please review.

Comparison is base (689585f) 65.51% compared to head (7bf61e7) 63.29%.

Files	Patch %	Lines
service/api/service_impl.go	0.00%	105 Missing ⚠️
...rsistence/sql/update_process_execution_from_rpc.go	0.00%	85 Missing ⚠️
persistence/sql/get_latest_process_execution.go	0.00%	22 Missing ⚠️
service/api/gin_handler.go	0.00%	22 Missing ⚠️
persistence/sql/common.go	80.51%	10 Missing and 5 partials ⚠️
persistence/sql/complete_execute.go	80.76%	3 Missing and 2 partials ⚠️
utils/util.go	81.25%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #97      +/-   ##
==========================================
- Coverage   65.51%   63.29%   -2.22%     
==========================================
  Files          77       80       +3     
  Lines        5849     6133     +284     
==========================================
+ Hits         3832     3882      +50     
- Misses       1775     2008     +233     
- Partials      242      243       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

longquanzheng

great job (esp the refactoring)

longquanzheng · 2023-11-19T19:27:43Z

engine/immediate_task_conrrent_processor.go

 		}
 	}

-	if w.checkResponseAndError(errToCheck, httpResp) {
+	if utils.CheckHttpResponseAndError(errToCheck, httpResp, w.logger) {


nit: try to avoid creating "utils" package as it will be hard to remember what are inside later. As a result it's hard for reuse.

Our pattern is create small package for specific area under common: https://github.com/xcherryio/xcherry/tree/main/common

E.g. urlautofix, ptr

for this one, it will be clearer if we create a package httperror (avoid http )and use as httperror.CheckReponseAndErrorr(err, httpResp, logger)

And
decision.ValidateDecision(...)

longquanzheng · 2023-11-19T19:30:06Z

utils/util.go

+	"github.com/xdblab/xdb/common/log/tag"
+)
+
+func CheckDecision(decision xdbapi.StateDecision) error {


actually the name could be improved to ValidateDecision which usually indicate to return error.

CheckXyz usually returns bool.

longquanzheng · 2023-11-19T19:44:45Z

persistence/data_models/data_models.go

+		UpdatingGlobalAttributesError  error
+	}
+
+	HandleStateDecisionRequest struct {


it seems like this could be an a structure inside the implementation package(sql) because the caller of persistence API doesn't need to understand

Will move them into sql/common.go.

longquanzheng · 2023-11-19T19:46:06Z

persistence/data_models/data_models.go

@@ -47,12 +47,31 @@ type (
 		NotExists bool
 	}



my mistake in the previous code review -- I think it's better to rename this file to request_response.go

Will move them into sql/common.go.

longquanzheng · 2023-11-19T19:47:38Z

persistence/data_models/data_models.go

@@ -277,6 +296,54 @@ type (
 	LoadGlobalAttributesResponse struct {
 		Response xdbapi.LoadGlobalAttributeResponse
 	}
+
+	UpdateProcessExecutionFromRpcRequest struct {


nit: maybe FromRpc -> ForRpc ?

longquanzheng · 2023-11-19T20:02:19Z

persistence/sql/common.go

+	// then gracefully complete the process regardless of the thread close type set in this state.
+	// Otherwise, handle the thread close type set in this state.
+
+	toGracefullyComplete := procExecWaitToComplete && len(sequenceMaps.PendingExecutionMap) == 0


just naming nit, maybe:

shouldGracefulComplete := procExecGracefulCompleteRequested && len(sequenceMaps.PendingExecutionMap) == 0

related to the issue: #66

GracefulCompleteRequested should make it more clear that there was a gracefulComplete decision was made before.

I think it will be better to make another PR for the issue, rather than partially renaming something in this PR to make naming inconsistent.

longquanzheng · 2023-11-19T20:13:33Z

persistence/sql/update_process_execution_from_rpc.go

+		return nil, err
+	}
+
+	// skip the writing operations on a closed process


Instead of silently skip this update, it's better to return an error(maybe conflict error) for this case.

users should be informed that the update is rejected otherwise they won't have the correct expectation for what has happened. (this is the behavior in iWF today without any extra code because Cadence/Temporal will throw error when sending signals to a closed workflow execution)

Will return StatusNotFound

longquanzheng · 2023-11-19T20:25:43Z

service/api/service_impl.go

+	}
+
+	if utils.CheckHttpResponseAndError(err, httpResp, s.logger) {
+		return nil, NewErrorWithStatus(


There could be more useful info that the caller need to get from the worker execution.

In the API, we have decided a workerErrorResponse, and we should try to extract it from the http body to caller.

See similar in iWF server: https://github.com/indeedeng/iwf/blob/main/service/common/rpc/invoke.go#L59

The reason is that worker may want to return some useful error type for caller to make different decision on.

Let's make another PR to handle it. Several places need to modify accordingly, not only here. Created #98.

zklgame force-pushed the add_rpc branch from 753fdb2 to a6bda71 Compare November 14, 2023 13:47

add RPC

c5ae8f2

zklgame force-pushed the add_rpc branch from a6bda71 to c5ae8f2 Compare November 14, 2023 13:51

zklgame requested a review from longquanzheng November 14, 2023 14:17

longquanzheng reviewed Nov 15, 2023

View reviewed changes

address comments

8f2b8b8

zklgame requested a review from longquanzheng November 16, 2023 00:45

zklgame added 2 commits November 16, 2023 23:08

u[date

7bf61e7

update

4de315a

longquanzheng reviewed Nov 19, 2023

View reviewed changes

zklgame added 2 commits November 20, 2023 11:03

address comments

1ea4113

delete utils

cbb8f08

zklgame requested a review from longquanzheng November 20, 2023 02:08

longquanzheng approved these changes Nov 20, 2023

View reviewed changes

zklgame merged commit 8a9b87b into main Nov 20, 2023
3 checks passed

zklgame deleted the add_rpc branch November 20, 2023 02:25

Add RPC #97

Add RPC #97

Conversation

zklgame commented Nov 14, 2023 • edited Loading

Why make this pull request?

What has changed

How to test this pull request?

Checklist before merge

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

zklgame Nov 16, 2023 • 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

codecov bot commented Nov 16, 2023 • edited Loading

Codecov Report

longquanzheng 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zklgame commented Nov 14, 2023 •

edited

Loading

zklgame Nov 16, 2023 •

edited

Loading

codecov bot commented Nov 16, 2023 •

edited

Loading