Skip to content

Conversation

Spikhalskiy
Copy link
Contributor

After #1438 WorkerOptions.Builder#setMaxConcurrentWorkflowTaskPollers(1) became an illegal configuration leading a worker to listen only on a sticky task queue. In this PR worker will override 1 to 2 and log a warning about it.

@Spikhalskiy Spikhalskiy self-assigned this Sep 22, 2022
@Spikhalskiy Spikhalskiy added this to the 1.17.0 milestone Sep 22, 2022
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

Heh, I ran into the same thing back when working on this in core

factoryOptions.getWorkflowHostLocalTaskQueueScheduleToStartTimeout();
}

int maxConcurrentWorkflowTaskPollers = options.getMaxConcurrentWorkflowTaskPollers();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I'd put this in WorkerOptions::Builder::validateAndBuildWithDefaults()...

Copy link
Contributor Author

@Spikhalskiy Spikhalskiy Sep 22, 2022

Choose a reason for hiding this comment

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

Yeah, I thought about it, because it looks like the right place, but decided against it.

  1. Some users may be building with just build. To catch them it will be a duplicate
  2. Users may not like validateAndBuildWithDefaults CHANGING their explicitly set values. If they read it somewhere afterward. It's not what this method should do. It should error out if the value specified by the user is incorrect. And we can't error out because of backward compatibility.

After considering it I decided to carry an incorrect but tolerable value over and correct it on a later stage in the Worker.

@Spikhalskiy Spikhalskiy enabled auto-merge (squash) September 23, 2022 15:18
@Spikhalskiy Spikhalskiy merged commit b6d65aa into temporalio:master Sep 23, 2022
@Spikhalskiy Spikhalskiy deleted the pollers-1-2 branch September 23, 2022 15:32
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.

4 participants