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

Use static membership for devserver #582

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

stephanos
Copy link
Contributor

@stephanos stephanos commented Jun 4, 2024

What was changed

Use static membership assignment for server services.

Why?

Fixes #632 and #633.

Checklist

  1. Closes

  2. How was this tested:

(1) used a script to verify the behavior

Before change:

go build ./cmd/temporal
Iteration 1/10
Error: 'Frontend is not healthy yet' found.
Killing the Temporal server with PID 21433...
go build ./cmd/temporal
Iteration 1/10
Killing the Temporal server with PID 21600...
Iteration 2/10
Error: 'Not enough hosts to serve the request' found.
Killing the Temporal server with PID 21613...

After change:

go build ./cmd/temporal
Iteration 1/10
Killing the Temporal server with PID 21190...
Iteration 2/10
Killing the Temporal server with PID 21198...
Iteration 3/10
Killing the Temporal server with PID 21206...
Iteration 4/10
Killing the Temporal server with PID 21214...
Iteration 5/10
Killing the Temporal server with PID 21222...
Iteration 6/10
Killing the Temporal server with PID 21230...
Iteration 7/10
Killing the Temporal server with PID 21237...
Iteration 8/10
Killing the Temporal server with PID 21243...
Iteration 9/10
Killing the Temporal server with PID 21251...
Iteration 10/10
Killing the Temporal server with PID 21257...
Script
#!/bin/bash

make build

SERVER_PID=0
kill_server() {
    if [ $SERVER_PID -ne 0 ]; then
        echo "Killing the Temporal server with PID $SERVER_PID..."
        kill $SERVER_PID
        wait $SERVER_PID 2>/dev/null
        SERVER_PID=0
        rm -rf server_output.log
    fi
}
trap kill_server EXIT

# Loop to run the start-stop sequence 10 times
for i in {1..10}; do
    echo "Iteration $i/10"

    ./temporal server start-dev --db-filename=/tmp/db > server_output.log 2>&1 &
    SERVER_PID=$!

    sleep 2

    if grep -q "Not enough hosts to serve the request" server_output.log; then
        echo "Error: 'Not enough hosts to serve the request' found."
        kill $SERVER_PID
        exit 1
    fi

    if grep -q "Frontend is not healthy yet" server_output.log; then
        echo "Error: 'Frontend is not healthy yet' found."
        kill $SERVER_PID
        exit 1
    fi

    kill_server

    sleep 1
done

(2) ran sdk-go integration tests against dev server

  1. Any docs updates needed?

@stephanos stephanos changed the base branch from main to cli-rewrite June 4, 2024 00:58
@temporalio temporalio deleted a comment from CLAassistant Jun 4, 2024
@temporalio temporalio deleted a comment from CLAassistant Jun 4, 2024
@stephanos stephanos force-pushed the static-membership-devserver branch from bfc1c5d to e4edef3 Compare June 4, 2024 01:01
@stephanos stephanos changed the title Use static membership for devserver Use static membership for devserver [do-not-merge] Jun 4, 2024
@mjameswh
Copy link
Contributor

mjameswh commented Jun 4, 2024

I expect this should make it possible to remove assignation of membership ports, which is a good thing.

	conf.RPC.MembershipPort = MustGetFreePort("127.0.0.1")

@stephanos stephanos force-pushed the static-membership-devserver branch from e4edef3 to 9f4908e Compare June 4, 2024 15:55
@stephanos stephanos changed the base branch from cli-rewrite to main August 9, 2024 16:13
@stephanos stephanos force-pushed the static-membership-devserver branch 2 times, most recently from 1c2aa5d to 96d9e10 Compare August 9, 2024 16:15
@stephanos stephanos force-pushed the static-membership-devserver branch from 96d9e10 to bb9e006 Compare August 9, 2024 16:17
@stephanos stephanos changed the title Use static membership for devserver [do-not-merge] Use static membership for devserver Aug 9, 2024
@stephanos stephanos marked this pull request as ready for review August 9, 2024 16:59
Copy link
Contributor

@dandavison dandavison left a comment

Choose a reason for hiding this comment

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

LGTM! I'm not previously familiar with this configuration but it looks reasonable to me and I can confirm that it fixes #632.

@stephanos stephanos merged commit 792b515 into temporalio:main Aug 9, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Server restarts in incorrect state when using file-backed persistence
4 participants