Change where a connection is deleted #19096
Conversation
github-actions bot added area/platform 
benmoriceau requested review from alovew, cgardens, colesnodgrass, gosusnp and jdpgrailsdev | .toList(); | ||
| | ||
| for (final UUID uuidToDelete : uuidsToDelete) { | ||
| connectionsHandler.deleteConnection(uuidToDelete); | ||
| } |
There was a problem hiding this comment.
You could instead call .forEach instead of calling toList and then iterating over that list.
There was a problem hiding this comment.
It is done on purpose in order to have the exception thrown by deleteConnection be thrown by the method (deleteConnection starts throwing in this PR).
| if (dontDeleteInTemporal < DONT_DELETE_IN_TEMPORAL_TAG_CURRENT_VERSION) { | ||
| if (workflowState.isDeleted()) { |
There was a problem hiding this comment.
Can combine these two if statements into one one to avoid one layer of nesting.
airbyte-server/src/main/java/io/airbyte/server/handlers/ConnectionsHandler.java Show resolved Hide resolved
| final int dontDeleteInTemporal = | ||
| Workflow.getVersion(DONT_DELETE_IN_TEMPORAL_TAG, Workflow.DEFAULT_VERSION, DONT_DELETE_IN_TEMPORAL_TAG_CURRENT_VERSION); | ||
| | ||
| if (dontDeleteInTemporal < DONT_DELETE_IN_TEMPORAL_TAG_CURRENT_VERSION && workflowState.isDeleted()) { |
There was a problem hiding this comment.
This is not placed in the right place, if should be on like 195. I moved the PR back to a draft.
| The build is actually successful, it just fail to report its status. |
…ate-delete-endpoint
| client.newWorkflowStub(ConnectionManagerWorkflow.class, getConnectionManagerName(connectionId)); | ||
| connectionManagerWorkflow.deleteConnection(); | ||
| } catch (final Exception e) { | ||
| log.warn("The workflow is not reachable when trying to cancel it"); |
There was a problem hiding this comment.
nit: can we log the exception?
There was a problem hiding this comment.
Opps, I forget that. Will update.
* Tmp * Move when the deletion is performed * Re-enable disable test * PR comments * Use cancel * rename * Fix test and version check position * Log exception
What
In ordert to fix a flaky test, we need to delete a connection as soon as the endpoint for the deletion is called.
This PR is moving the deletion from a temporal activity to the API call.
This is addressing #18266
Another PR is needed in order to remove the extra signal in case there is a version issue. Since this is a tail activity call, it can be done right after this PR is moved to cloud.