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

Bump server to 1.26.2, enable HTTP by default #734

Open
wants to merge 4 commits into
base: next-server
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ require (
github.com/temporalio/ui-server/v2 v2.31.2
go.temporal.io/api v1.43.0
go.temporal.io/sdk v1.31.0
go.temporal.io/server v1.26.2-rc.0
go.temporal.io/server v1.26.2
google.golang.org/grpc v1.67.1
google.golang.org/protobuf v1.35.1
gopkg.in/yaml.v3 v3.0.1
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -358,8 +358,8 @@ go.temporal.io/api v1.43.0 h1:lBhq+u5qFJqGMXwWsmg/i8qn1UA/3LCwVc88l2xUMHg=
go.temporal.io/api v1.43.0/go.mod h1:1WwYUMo6lao8yl0371xWUm13paHExN5ATYT/B7QtFis=
go.temporal.io/sdk v1.31.0 h1:CLYiP0R5Sdj0gq8LyYKDDz4ccGOdJPR8wNGJU0JGwj8=
go.temporal.io/sdk v1.31.0/go.mod h1:8U8H7rF9u4Hyb4Ry9yiEls5716DHPNvVITPNkgWUwE8=
go.temporal.io/server v1.26.2-rc.0 h1:jsWR2UzhPQ+UgDfqNHT6h6ZjzGTcw72i9KNkSNS0jN8=
go.temporal.io/server v1.26.2-rc.0/go.mod h1:tgY+4z/PuIdqs6ouV1bT90RWSWfEioWkzmrNrLYLUrk=
go.temporal.io/server v1.26.2 h1:vDW11lxslYPlGDbQklWi/tqbkVZ2ExtRO1jNjvZmUUI=
go.temporal.io/server v1.26.2/go.mod h1:tgY+4z/PuIdqs6ouV1bT90RWSWfEioWkzmrNrLYLUrk=
go.temporal.io/version v0.3.0 h1:dMrei9l9NyHt8nG6EB8vAwDLLTwx2SvRyucCSumAiig=
go.temporal.io/version v0.3.0/go.mod h1:UA9S8/1LaKYae6TyD9NaPMJTZb911JcbqghI2CBSP78=
go.uber.org/atomic v1.5.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ=
Expand Down
2 changes: 1 addition & 1 deletion temporalcli/commands.gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -1830,7 +1830,7 @@ func NewTemporalServerStartDevCommand(cctx *CommandContext, parent *TemporalServ
s.Command.Flags().StringVarP(&s.DbFilename, "db-filename", "f", "", "Path to file for persistent Temporal state store. By default, Workflow Executions are lost when the server process dies.")
s.Command.Flags().StringArrayVarP(&s.Namespace, "namespace", "n", nil, "Namespaces to be created at launch. The \"default\" Namespace is always created automatically.")
s.Command.Flags().IntVarP(&s.Port, "port", "p", 7233, "Port for the front-end gRPC Service.")
s.Command.Flags().IntVar(&s.HttpPort, "http-port", 0, "Port for the HTTP API service. Default is off.")
s.Command.Flags().IntVar(&s.HttpPort, "http-port", 0, "Port for the HTTP API service. Defaults to a random free port.")
s.Command.Flags().IntVar(&s.MetricsPort, "metrics-port", 0, "Port for '/metrics'. Default is off.")
s.Command.Flags().IntVar(&s.UiPort, "ui-port", 0, "Port for the Web UI. Default is '--port' value + 1000.")
s.Command.Flags().BoolVar(&s.Headless, "headless", false, "Disable the Web UI.")
Expand Down
11 changes: 9 additions & 2 deletions temporalcli/commands.server.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,14 @@ func (t *TemporalServerStartDevCommand) run(cctx *CommandContext, args []string)
if err := devserver.CheckPortFree(opts.FrontendIP, opts.FrontendPort); err != nil {
return fmt.Errorf("can't set frontend port %d: %w", opts.FrontendPort, err)
}
if err := devserver.CheckPortFree(opts.FrontendIP, opts.FrontendHTTPPort); err != nil {
return fmt.Errorf("can't set frontend HTTP port %d: %w", opts.FrontendHTTPPort, err)

// Grab a free port for HTTP ahead-of-time so we know what port is selected
if opts.FrontendHTTPPort == 0 {
opts.FrontendHTTPPort = devserver.MustGetFreePort(opts.FrontendIP)
} else {
if err := devserver.CheckPortFree(opts.FrontendIP, opts.FrontendHTTPPort); err != nil {
return fmt.Errorf("can't set frontend HTTP port %d: %w", opts.FrontendHTTPPort, err)
}
}
// Setup UI
if !t.Headless {
Expand Down Expand Up @@ -152,6 +158,7 @@ func (t *TemporalServerStartDevCommand) run(cctx *CommandContext, args []string)

cctx.Printer.Printlnf("CLI %v\n", VersionString())
cctx.Printer.Printlnf("%-8s %v:%v", "Server:", toFriendlyIp(opts.FrontendIP), opts.FrontendPort)
cctx.Printer.Printlnf("%-8s %v:%v", "HTTP:", toFriendlyIp(opts.FrontendIP), opts.FrontendHTTPPort)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little concerned about this part. The HTTP API is not considered stable and is not really documented. So listing this as though it is I think is a bit rough. Can we only display this when the port was given explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... for metrics we print even if the port isn't specified. Let's keep this consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The concern is not about whether the port is specified, it's about promoting the HTTP API. Metrics is stable/GA and documented w/ a cloud equivalent, HTTP API is not. We should not by default encourage use of the HTTP API in dev server at this time until it is a documented product available to all cloud users by default. But if a user explicitly sets an option, that's different/understandable, though I would also be ok just not showing it at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if !t.Headless {
cctx.Printer.Printlnf("%-8s http://%v:%v%v", "UI:", toFriendlyIp(opts.UIIP), opts.UIPort, opts.PublicPath)
}
Expand Down
18 changes: 14 additions & 4 deletions temporalcli/commands.server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,26 +25,29 @@ import (

func TestServer_StartDev_Simple(t *testing.T) {
port := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1"))
httpPort := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these test updates needed? Can't we just let it choose the random default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not, I left it here in prep for when we make the HTTP port fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's a reasonable assumption/expectation yet. But so long as there's some code path somewhere that is covering the random part, all good, even if it's not needed.

startDevServerAndRunSimpleTest(
t,
// TODO(cretz): Remove --headless when
// https://github.com/temporalio/ui/issues/1773 fixed
[]string{"server", "start-dev", "-p", port, "--headless"},
[]string{"server", "start-dev", "-p", port, "--http-port", httpPort, "--headless"},
"127.0.0.1:"+port,
)
}

func TestServer_StartDev_IPv4Unspecified(t *testing.T) {
port := strconv.Itoa(devserver.MustGetFreePort("0.0.0.0"))
httpPort := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1"))
startDevServerAndRunSimpleTest(
t,
[]string{"server", "start-dev", "--ip", "0.0.0.0", "-p", port, "--headless"},
[]string{"server", "start-dev", "--ip", "0.0.0.0", "-p", port, "--http-port", httpPort, "--headless"},
"0.0.0.0:"+port,
)
}

func TestServer_StartDev_SQLitePragma(t *testing.T) {
port := strconv.Itoa(devserver.MustGetFreePort("0.0.0.0"))
httpPort := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1"))
dbFilename := filepath.Join(os.TempDir(), "devserver-sqlite-pragma.sqlite")
defer func() {
_ = os.Remove(dbFilename)
Expand All @@ -55,7 +58,9 @@ func TestServer_StartDev_SQLitePragma(t *testing.T) {
t,
[]string{
"server", "start-dev",
"-p", port, "--headless",
"-p", port,
"--http-port", httpPort,
"--headless",
"--db-filename", dbFilename,
"--sqlite-pragma", "journal_mode=WAL",
"--sqlite-pragma", "synchronous=NORMAL",
Expand All @@ -76,12 +81,14 @@ func TestServer_StartDev_IPv6Unspecified(t *testing.T) {
}

port := strconv.Itoa(devserver.MustGetFreePort("::"))
httpPort := strconv.Itoa(devserver.MustGetFreePort("::"))
startDevServerAndRunSimpleTest(
t,
[]string{
"server", "start-dev",
"--ip", "::", "--ui-ip", "::1",
"-p", port,
"--http-port", httpPort,
"--ui-port", strconv.Itoa(devserver.MustGetFreePort("::")),
"--http-port", strconv.Itoa(devserver.MustGetFreePort("::")),
"--metrics-port", strconv.Itoa(devserver.MustGetFreePort("::"))},
Expand Down Expand Up @@ -138,9 +145,10 @@ func TestServer_StartDev_ConcurrentStarts(t *testing.T) {

// Start in background, then wait for client to be able to connect
port := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1"))
httpPort := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1"))
resCh := make(chan *CommandResult, 1)
go func() {
resCh <- h.Execute("server", "start-dev", "-p", port, "--headless", "--log-level", "never")
resCh <- h.Execute("server", "start-dev", "-p", port, "--http-port", httpPort, "--headless", "--log-level", "never")
}()

// Try to connect for a bit while checking for error
Expand Down Expand Up @@ -191,11 +199,13 @@ func TestServer_StartDev_WithSearchAttributes(t *testing.T) {

// Start in background, then wait for client to be able to connect
port := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1"))
httpPort := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1"))
resCh := make(chan *CommandResult, 1)
go func() {
resCh <- h.Execute(
"server", "start-dev",
"-p", port,
"--http-port", httpPort,
"--headless",
"--search-attribute", "search-attr-1=Int",
"--search-attribute", "search-attr-2=kEyWoRdLiSt",
Expand Down
1 change: 0 additions & 1 deletion temporalcli/commands_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,6 @@ func StartDevServer(t *testing.T, options DevServerOptions) *DevServer {
d.Options.DynamicConfigValues["frontend.workerVersioningWorkflowAPIs"] = true
d.Options.DynamicConfigValues["worker.buildIdScavengerEnabled"] = true
d.Options.DynamicConfigValues["frontend.enableUpdateWorkflowExecution"] = true
d.Options.DynamicConfigValues["system.enableNexus"] = true
d.Options.DynamicConfigValues["frontend.MaxConcurrentBatchOperationPerNamespace"] = 1000
d.Options.DynamicConfigValues["frontend.namespaceRPS.visibility"] = 100
d.Options.DynamicConfigValues["system.clusterMetadataRefreshInterval"] = 100 * time.Millisecond
Expand Down
7 changes: 3 additions & 4 deletions temporalcli/commandsgen/commands.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1647,9 +1647,8 @@ commands:
default: 7233
- name: http-port
type: int
description: |
Port for the HTTP API service.
Default is off.
description: Port for the HTTP API service. Defaults to a random free port.
Copy link
Member

@cretz cretz Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have any way to disable the HTTP API service? I can't think of a good reason at this time, so can probably not worry about it for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this but don't think it's a concern if it defaults to a random port. We can always add a --no-http flag if we wanted later.

default: 0
- name: metrics-port
type: int
description: |
Expand Down Expand Up @@ -3502,4 +3501,4 @@ option-sets:
description: |
An external Nexus Endpoint that receives forwarded Nexus requests.
May be used as an alternative to `--target-namespace` and
`--target-task-queue`.
`--target-task-queue`.
5 changes: 1 addition & 4 deletions temporalcli/devserver/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,6 @@ func (s *StartOptions) buildServerOptions() ([]temporal.ServerOption, error) {
dynConf[dynamicconfig.HistoryCacheHostLevelMaxSize.Key()] = 8096
// Up default visibility RPS
dynConf[dynamicconfig.FrontendMaxNamespaceVisibilityRPSPerInstance.Key()] = 100
// This doesn't enable Nexus but it is required for Nexus to work and simplifies the experience.
// NOTE that the URL scheme is fixed to HTTP since the dev server doesn't support TLS at the time of writing.
dynConf[nexusoperations.CallbackURLTemplate.Key()] = fmt.Sprintf(
"http://%s:%d/namespaces/{{.NamespaceName}}/nexus/callback", MaybeEscapeIPv6(s.FrontendIP), s.FrontendHTTPPort)
Expand Down Expand Up @@ -359,9 +358,7 @@ func (s *StartOptions) buildServiceConfig(frontend bool) config.Service {
if frontend {
conf.RPC.GRPCPort = s.FrontendPort
conf.RPC.BindOnIP = s.FrontendIP
if s.FrontendHTTPPort > 0 {
conf.RPC.HTTPPort = s.FrontendHTTPPort
}
conf.RPC.HTTPPort = s.FrontendHTTPPort
} else {
conf.RPC.GRPCPort = MustGetFreePort(s.FrontendIP)
conf.RPC.BindOnIP = s.FrontendIP
Expand Down
Loading