pjfanning opened a new pull request, #954:
URL: https://github.com/apache/pekko-management/pull/954
### Motivation
Follow-up to #907.
The `ec2ClientUsed` / `ecsClientUsed` flags added in #907 are set at the
*top* of the lazy client initialiser, before the client exists:
```scala
private lazy val ec2Client: AmazonEC2 = {
ec2ClientUsed = true // <-- before the client is built
...
builder.build()
}
```
A Scala `lazy val` whose initialiser throws stays uninitialised and is
re-evaluated on the next access. So if `builder.build()` fails, or the custom
`client-config` FQCN cannot be instantiated (`throw new Exception(s"Could not
create instance of '$fqcn'", ex)`), we end up with `ec2ClientUsed == true` and
no client. The `PhaseServiceUnbind` task then calls `ec2Client.shutdown()`,
re-enters the initialiser and throws again — which is precisely the
shutdown-time client creation the flag was added to avoid (see @He-Pin's review
comment on #907). Setting the flag before `builder.build()` returns also leaves
a window where shutdown observes `true` while another thread is still
constructing the client.
Separately, the two collection changes in #907 are not the optimisations the
description claimed:
- `taskArns.grouped(100).toSeq` — `IterableOnceOps.toSeq` is
`immutable.Seq.from(this)`, i.e. strict, and `List` is the default
`immutable.Seq`. This materialises exactly like the `.toList` it replaced.
- EC2 `getInstances` — `java.util.List.asScala` yields a strict
`mutable.Buffer`, not a lazy view, so `flatMap` then `map` build two
intermediate buffers before `.toList`. No fewer allocations than the original.
Both paginated loops also do `accumulator ++ page` per page (`List` in EC2,
`immutable.Seq` in ECS), which is O(pages²) copying.
### Modification
- Replace both `@volatile var` flags with an `AtomicReference` that is
populated **after** `builder.build()` succeeds. The shutdown task closes
whatever client it finds there and returns `Done` when it is null, so it never
touches the lazy val.
- EC2 `getInstances`: use `.view` so `flatMap`/`map` are genuinely lazy, and
accumulate into a `Vector`.
- ECS `describeTasks`: `flatMap` straight off the `grouped` iterator instead
of materialising the groups first.
- ECS `listTaskArns`: accumulate into a `Vector` so appends are amortised
constant time.
- New `Ec2ClientShutdownSpec` and `EcsResolveTasksSpec`.
`EcsServiceDiscovery.resolveTasks` is relaxed from `private` to `private[ecs]`
so the latter can drive it — a new method in bytecode, so MiMa-clean.
### Result
Shutdown never creates, or re-attempts to create, an AWS client — including
after a failed creation. Pagination is linear in the number of pages rather
than quadratic, and the EC2 chain allocates one list instead of three.
### Tests
- `sbt "discovery-aws-api/test"` — 8 succeeded, 0 failed.
- Directional check: `sbt "discovery-aws-api/testOnly
*Ec2ClientShutdownSpec"` with `Ec2TagBasedServiceDiscovery.scala` reverted to
`main` — *"should not re-attempt EC2 client creation during shutdown when
creation previously failed"* **FAILED** (2 configuration attempts instead of
1). Passes with the fix.
- `sbt "+discovery-aws-api/mimaReportBinaryIssues"` — success.
- `sbt "discovery-aws-api/scalafmt" "discovery-aws-api/Test/scalafmt"` and
`sbt headerCreateAll` — clean. `sortImports` is not a task in this build (only
`javafmtSortImports`); no Java sources changed.
- Not exercised against live AWS — no account available. The specs stub
`AbstractAmazonECS` and `ClientConfiguration` so no credentials, region or
network access are needed.
### References
Refs #907
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
