Handle multiple bindings between app and service#1820
Handle multiple bindings between app and service#1820theghost5800 wants to merge 2 commits intomasterfrom
Conversation
JIRA:LMCROSSITXSADEPLOY-3409
theghost5800
force-pushed
the
handleMultipleBindings
branch
from
2ccc380 to
f5ed5cc
Compare
| Optional.ofNullable(deleteSingleServiceBinding(guid)) | ||
| .ifPresent(jobIds::add); | ||
| } catch (RuntimeException e) { | ||
| errors.add(e); |
There was a problem hiding this comment.
Why not fast-failing immediately, but collecting the errors? If there is some regression with a service broker, won't this produce too much noise?
There was a problem hiding this comment.
The reason for this change is due to common from claude review that if we use this codeblock
return Flux.fromIterable(guids)
.map(UUID::toString)
.flatMap(guid -> delegate.serviceBindingsV3().delete(...))
.collectList()
.block();
it will try to run all deletion requests concurrently and it's possible to lose track of other delete request responses. It's interesting which approach is better.
| try { | ||
| Optional.ofNullable(deleteSingleServiceBinding(guid)) | ||
| .ifPresent(jobIds::add); | ||
| } catch (RuntimeException e) { |
There was a problem hiding this comment.
Why catching so generic exception here? Can't we use AbstractCloudFoundryException?
| List<String> jobIds = new ArrayList<>(); | ||
| List<RuntimeException> errors = new ArrayList<>(); |
There was a problem hiding this comment.
These two parameters are in-parameters which is not a best practice. Can you split the method to just return a List of some results instead?
| protected AsyncExecutionState checkServiceBindingOperationState(CloudServiceBinding serviceBinding, ProcessContext context) { | ||
| ServiceCredentialBindingOperation lastOperation = serviceBinding.getServiceBindingOperation(); | ||
| protected AsyncExecutionState checkServiceBindingOperationState(List<CloudServiceBinding> serviceBindings, ProcessContext context) { | ||
| CloudServiceBinding failedServiceBinding = null; |
There was a problem hiding this comment.
Why only the last failed binding is tracked and not all? Is it always the latest?
There was a problem hiding this comment.
I think we should rename these type of classes so they reflect the class is used to monitor multiple bindings between app and service
| public AsyncExecutionState execute(ProcessContext context) { | ||
| List<String> jobIds = context.getVariable(Variables.SERVICE_UNBINDING_JOB_IDS); | ||
| if (jobIds.isEmpty()) { | ||
| return super.execute(context); |
There was a problem hiding this comment.
Won't this produce a null pointer exception because
context.setVariable(Variables.SERVICE_UNBINDING_JOB_ID, jobId);
is not set if it is a first execution of this step?
| .getState() == ServiceCredentialBindingOperation.State.SUCCEEDED) | ||
| .max(Comparator.comparing(binding -> binding.getMetadata() | ||
| .getCreatedAt())) | ||
| .orElseGet(() -> bindings.isEmpty() ? null : bindings.get(0)); |
There was a problem hiding this comment.
I guess we will never get here, but isn't it more correctly at least to return the latest created binding instead of bindings.get(0)? Or just throw an exception?
|
3 participants
JIRA:LMCROSSITXSADEPLOY-3409