markap14 commented on PR #11231: URL: https://github.com/apache/nifi/pull/11231#issuecomment-4425376094
@kevdoran thanks for the careful walk-through — really appreciated the per-method classification and the catalog of redundant syncs. I pushed `146f89be67c` with all of your suggestions implemented: **1. Start paths (3 sites)** — dropped `SYNC_WITH_PROVIDER`: - `FlowController#startConnector(ConnectorNode)` — removed the pre-start `getConnector(..., SYNC_WITH_PROVIDER)` entirely; the start path doesn't read provider state. - `FlowController#onFlowInitialized` deferred-start loop — the existence check is now `LOCAL_ONLY`. - `StandardConnectorDAO#startConnector(String)` — switched to `LOCAL_ONLY`. **2. `Objects.requireNonNull(syncMode)`** — added to both `StandardConnectorRepository.getConnector(String, ConnectorSyncMode)` and `getConnectors(ConnectorSyncMode)` so a missing mode fails fast. **3. Rename path layering** — `StandardConnectorRepository.updateConnector(connector, name)` now does its own `provider.load` (falling back to a locally-built working configuration when the provider has no record) before `provider.save`, mirroring `configureConnector`. `StandardConnectorDAO.updateConnector` drops to `LOCAL_ONLY` accordingly. Every DAO write path is now uniformly `LOCAL_ONLY` while the no-clobber guarantee for out-of-band provider changes is preserved by the repository. **4. `StandardNiFiServiceFacade` callers** — converted via the existing `getConnector(id, ConnectorSyncMode)` overload: - *Locators (6)*: `locateConnectorProcessor`, `locateConnectorControllerService`, `getConnectorFlow`, `getConnectorProcessGroupStatus`, `getConnectorControllerServices`, `searchConnector` — all `LOCAL_ONLY` (only touch active flow context). - *State check (1)*: `verifyDrainConnector` — `LOCAL_ONLY`. - *Post-mutation re-reads (14)*: the inside-revision-block and outside-revision-block reads in `createConnector`, `updateConnector` (rename), `scheduleConnector`, `drainConnector`, `cancelConnectorDrain`, `updateConnectorConfigurationStep`, `applyConnectorUpdate`, and `discardConnectorUpdate` — all `LOCAL_ONLY`. The mutation already round-tripped the provider; the response-DTO read no longer triggers another load. - *Public reads kept on SYNC*: the four "serialize for REST response" sites you flagged — `getConnectors()`, `getConnector(id)`, `getConnectorConfigurationSteps`, `getConnectorConfigurationStep`. The pre-delete read in `deleteConnector` also stays on SYNC for response-DTO symmetry. **5. Bonus — `ConnectorAuditor` AOP advice (5 sites)** — these AOP `@Around` methods on `removeConnector`, `startConnector`, `stopConnector`, `updateConnectorConfigurationStep`, and `applyConnectorUpdate` were calling the no-arg `connectorDAO.getConnector(id)` (which defaulted to SYNC) just to capture local audit metadata around every mutation. Without changing them, every mutation would still pay one provider round trip via the auditor even though the DAO method itself is now `LOCAL_ONLY`. Switched all five to `LOCAL_ONLY`. **6. `addConnector(connector)` syncing** — agree that's fine; nothing in the diff calls it after an upstream provider sync, so no duplicate work. Build and tests are green: - `nifi-framework-core` — 543 tests, 0 failures. - `nifi-web-api` — 529 tests, 0 failures (added `testGetConnectorLocalOnly` and `testGetConnectorLocalOnlyWithNonExistentId` for the new DAO overload, updated stubs/verifies in `StandardNiFiServiceFacadeTest` for the seven Group A sites that now pass `ConnectorSyncMode.LOCAL_ONLY`). - `checkstyle:check pmd:check` clean on both modules. -- 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]
