Apply recent API changes

[ChrisKujawa]

Description

Return type is not necessary, since return was always true, which is why it has been removed from the API.

Related issues

Related to changes made here camunda/camunda@b4e0e05

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

• The changes are backwards compatibility with previous versions
• If it fixes a bug then PRs are created to backport the fix

Testing:

• There are unit/integration tests that verify all acceptance criterias of the issue
• New tests are written to ensure backwards compatibility with further versions
• The behavior is tested manually

Documentation:

• Javadoc has been written
• The documentation is updated

[github-actions]

Test Results

  47 files  ±0    47 suites  ±0   2m 4s ⏱️ - 1m 10s
117 tests ±0  116 ✔️ ±0  1 💤 ±0  0 ❌ ±0 
374 runs  ±0  371 ✔️ ±0  3 💤 ±0  0 ❌ ±0 

Results for commit bb4ba14. ± Comparison against base commit 5bc5b7f.

♻️ This comment has been updated with latest results.

[remcowesterhoud]

@Zelldon It seems there are some failing tests. I dug a bit into it and found out that there could be multiple pendingResponses for a single request.

This is unexpected to me, would you agree?

[remcowesterhoud]

Maybe to explain why the tests are failing. We are keeping track of a map of requests

  Long registerNewRequest(
      final Class<? extends GeneratedMessageV3> requestType,
      final StreamObserver<?> responseObserver) {
    final long currentRequestId = requestIdGenerator.incrementAndGet();
    requestMap.put(currentRequestId, new Request(requestType, responseObserver));
    return currentRequestId;
  }

When a response is written we remove the request from this map and send the response to the client

  @Override
  public void tryWriteResponse(final int requestStreamId, final long requestId) {
    if (rejectionType != RejectionType.NULL_VAL) {
      final Status rejectionResponse =
          responseMapper.createRejectionResponse(rejectionType, intent, rejectionReason);
      final Request request = gatewayRequestStore.removeRequest(requestId);
      sendError(request, rejectionResponse);
      return;
    }

    try {
      final Request request = gatewayRequestStore.removeRequest(requestId);
      final GeneratedMessageV3 response =
          responseMapper.map(request.requestType(), valueBufferView, key, intent);
      sendResponse(request, response);
    } catch (final Exception e) {
      throw new RuntimeException(e);
    }
  }

Since there is multiple responses for a single request we run into an NPE. We could fix it by not sending a response if we can't find the corresponding request. But I'd like to make sure this isn't a bug in Zeebe.

[ChrisKujawa]

@remcowesterhoud I think it is a bug in zeebe. https://github.com/camunda/zeebe/blob/main/stream-platform/src/main/java/io/camunda/zeebe/stream/impl/ProcessingStateMachine.java#L356

It is not really problematic, since the response is not sent twice if it was already sent, but yeah something which we should clean up.

I guess we should use a hashset instead of a list in the ProcessingStateMachine \cc @oleschoenburg

[ChrisKujawa]

Thanks for @remcowesterhoud checking the failures. I'm currently on FTO so have not time to fix it today.

[lenaschoenburg]

Fix is prepared: camunda/camunda#11892, I guess we can merge this tomorrow.

[ChrisKujawa]

Thanks to @oleschoenburg the CI is green again, I have rerun the jobs @remcowesterhoud I think we can merge it now.

[remcowesterhoud]

Perfect, thanks both!

bors merge

[ghost]

Build succeeded:

• Test summary