Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java sdk batchCheck does not do a batch check #127

Open
6 tasks done
lucasmarcelli opened this issue Nov 22, 2024 · 4 comments
Open
6 tasks done

Java sdk batchCheck does not do a batch check #127

lucasmarcelli opened this issue Nov 22, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@lucasmarcelli
Copy link

lucasmarcelli commented Nov 22, 2024

Checklist

  • I have looked into the README and have not found a suitable solution or answer.
  • I have looked into the documentation and have not found a suitable solution or answer.
  • I have searched the issues and have not found a suitable solution or answer.
  • I have upgraded to the latest version of OpenFGA and the issue still persists.
  • I have searched the Slack community and have not found a suitable solution or answer.
  • I agree to the terms within the OpenFGA Code of Conduct.

Description

Not sure if this is a bug per se, but the batchCheck function is not hitting the batch check endpoint on FGA. Rather, it's making several individual requests in parallel. If we make 30 checks in a batch check, with the default settings, it makes up to 10 parallel requests of single batch checks at a time until all 30 are checked.

We were having performance issues and noticed that this is happening, with context timeouts being hit after the 14th or 15th tuple check and the rest coming back null. We were able to mitigate this by increasing our db memory.

Is the way this works intentional? I'm planning to try hitting the actual batch endpoint in openfga manually to see if the performance is improved at all, but wanted to check here as well.

public CompletableFuture<List<ClientBatchCheckResponse>> batchCheck(
List<ClientCheckRequest> requests, ClientBatchCheckOptions batchCheckOptions)
throws FgaInvalidParameterException {
configuration.assertValid();
configuration.assertValidStoreId();
var options = batchCheckOptions != null
? batchCheckOptions
: new ClientBatchCheckOptions().maxParallelRequests(DEFAULT_MAX_METHOD_PARALLEL_REQS);
if (options.getAdditionalHeaders() == null) {
options.additionalHeaders(new HashMap<>());
}
options.getAdditionalHeaders().putIfAbsent(CLIENT_METHOD_HEADER, "BatchCheck");
options.getAdditionalHeaders()
.putIfAbsent(CLIENT_BULK_REQUEST_ID_HEADER, randomUUID().toString());
int maxParallelRequests = options.getMaxParallelRequests() != null
? options.getMaxParallelRequests()
: DEFAULT_MAX_METHOD_PARALLEL_REQS;
var executor = Executors.newScheduledThreadPool(maxParallelRequests);
var latch = new CountDownLatch(requests.size());
var responses = new ConcurrentLinkedQueue<ClientBatchCheckResponse>();
final var clientCheckOptions = options.asClientCheckOptions();
Consumer<ClientCheckRequest> singleClientCheckRequest =
request -> call(() -> this.check(request, clientCheckOptions))
.handleAsync(ClientBatchCheckResponse.asyncHandler(request))
.thenAccept(responses::add)
.thenRun(latch::countDown);
try {
requests.forEach(request -> executor.execute(() -> singleClientCheckRequest.accept(request)));
latch.await();
return CompletableFuture.completedFuture(new ArrayList<>(responses));
} catch (Exception e) {
return CompletableFuture.failedFuture(e);
}
}

Expectation

batchCheck should hit the batch endpoint in openfga.

Reproduction

  1. Make a batch check
  2. Observe that the URL being hit by the package is not the batch check endpoint, but is multiple requests to the single check endpoint

OpenFGA SDK version

0.4.x

OpenFGA version

1.5.9

SDK Configuration

i'm not sure what this means

Logs

No response

References

No response

@lucasmarcelli lucasmarcelli added the bug Something isn't working label Nov 22, 2024
@ewanharris
Copy link
Member

Hey @lucasmarcelli,

We're currently in the process of working to update the SDKs to use the BatchCheck API endpoint. So, the BatchCheck method is currently still the SDK wrapper method that makes multiple Check calls in parallel.

@lucasmarcelli
Copy link
Author

Thanks. Is there a task/issue?

@lucasmarcelli
Copy link
Author

I ask because I may have capacity to make this change in the java SDK if nobody has picked it up yet.

@ewanharris
Copy link
Member

@lucasmarcelli thanks for that offer!

We don't currently have this work broken down yet as we're finalizing our plans for it (we plan to maintain the same API interface and map the request/response internally). But I've raised your interest to the team so that we can be sure to let you know when we have that issue filed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

2 participants