Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement temporal.api.workflowservice.v1.WorkflowService/DescribeNamespace in test-server #1158

Closed
Tracked by #1094
seregazhuk opened this issue Apr 20, 2022 · 6 comments · Fixed by #1160
Closed
Tracked by #1094
Assignees
Labels
Milestone

Comments

@seregazhuk
Copy link

To start using test-sever in PHP-SDK we need the following functionality.

@Spikhalskiy
Copy link
Contributor

Spikhalskiy commented Apr 20, 2022

Other SDK successfully use test server without this functionality. If it's currently a prerequisite for using test server in PHP SDK, it shouldn't be. SDKs themselves never call describeNamespace, PHP shouldn't be different.

@rustatian
Copy link

rustatian commented Apr 20, 2022

Other SDK successfully use test server without this functionality. If it's currently a prerequisite for using test server in PHP SDK, it shouldn't be. SDKs themselves never call describeNamespace, PHP shouldn't be different.

Hey @Spikhalskiy, Could you please point me to a doc or manual with a testing-server limitations? It would be great to see what API it doesn't support.

@Spikhalskiy
Copy link
Contributor

This class: https://github.com/temporalio/sdk-java/blob/1816ad2499b8e98a20d158a8f0a4c64bb41a9a09/temporal-test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowService.java
implements WorkflowService, all methods that are not overridden will throw an UNIMPLEMENTED exception.

Currently, it's

  registerNamespace
  describeNamespace
  listNamespaces
  updateNamespace
  deprecateNamespace
  getWorkflowExecutionHistoryReverse
  resetWorkflowExecution
  listWorkflowExecutions
  listArchivedWorkflowExecutions
  scanWorkflowExecutions
  countWorkflowExecutions
  getSearchAttributes
  resetStickyTaskQueue
  describeTaskQueue
  getClusterInfo
  getSystemInfo
  listTaskQueuePartitions

Some notes:
registerNamespace is not needed for Test Server, you can just use any namespace name without registering with it
resetStickyTaskQueue probably should be implemented in a trivial way and just return silently without an exception. All SDKs just ignore any exception from the sticky queue reset, because it's not too important.

@rustatian
Copy link

rustatian commented Apr 20, 2022

@Spikhalskiy Great, thank you for the help.

However, this is not a PHP-SDK error, as I figured out.
I have created a simple worker in go-sdk (I'm not a Java dev, so forgive me for the snippet in Go), without any activities or workflows registered. Here is the sample:

package main

import (
	"log"

	"go.temporal.io/sdk/client"
	"go.temporal.io/sdk/worker"
)

func main() {
	// The client and worker are heavyweight objects that should be created once per process.
	c, err := client.NewClient(client.Options{
		HostPort: client.DefaultHostPort,
	})
	if err != nil {
		log.Fatalln("Unable to create client", err)
	}
	defer c.Close()

	w := worker.New(c, "greetings", worker.Options{})

	err = w.Run(worker.InterruptCh()) <-------- ERROR is here
	if err != nil {
		log.Fatalln("Unable to start worker", err)
	}
}

If you run this code snippet with a test-server, you'll get an error: 2022/04/20 18:16:05 Unable to start worker Method temporal.api.workflowservice.v1.WorkflowService/DescribeNamespace is unimplemented

EDIT: test-server version is: 1.10.0

@Spikhalskiy
Copy link
Contributor

Spikhalskiy commented Apr 20, 2022

Good catch, you are right. It was added to Go SDK here: uber-go/cadence-client#196
It's actually a discrepancy, we should either remove it from Go SDK or add it to other SDKs. Java, Typescript and I guess Python SDKs don't do it right now. We will discuss it. I don't see too much reason to make this explicit all.
But it looks like we should implement some stub describeNamespace in the test server just for the sake of compatibility with older versions of Go SDK.

@rustatian
Copy link

Got u, thanks 🤝 Looking forward to the solution 👍🏻

Spikhalskiy added a commit to Spikhalskiy/java-sdk that referenced this issue Apr 21, 2022
Spikhalskiy added a commit to Spikhalskiy/java-sdk that referenced this issue Apr 21, 2022
@Spikhalskiy Spikhalskiy self-assigned this Apr 21, 2022
@Spikhalskiy Spikhalskiy added this to the 1.11.0 milestone Apr 21, 2022
Spikhalskiy added a commit to Spikhalskiy/java-sdk that referenced this issue Apr 21, 2022
Spikhalskiy added a commit to Spikhalskiy/java-sdk that referenced this issue Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants