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

Fail fast if query has multiple statements #57

Closed
chadlwilson opened this issue Nov 9, 2021 · 3 comments
Closed

Fail fast if query has multiple statements #57

chadlwilson opened this issue Nov 9, 2021 · 3 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers size:XS Extra small items

Comments

@chadlwilson
Copy link
Collaborator

Context / Goal

Technically the current implementation probably allows multiple statements to be expressed inside the queries, and will be executed.

This shouldn't be allowed and should ideally fail at execution, however as noted in #56, we don't actually have capacity to do SQL parsing - this is delegated to the database and/or drivers.

Expected Outcome

Out of Scope

Additional context / implementation notes

@chadlwilson chadlwilson added the bug Something isn't working label Nov 9, 2021
@chadlwilson chadlwilson added good first issue Good for newcomers size:XS Extra small items labels Nov 9, 2021
@jiawen-tw jiawen-tw self-assigned this Mar 2, 2022
@jiawen-tw
Copy link
Contributor

jiawen-tw commented Mar 3, 2022

  • Observed that the number of sql statements corresponds to the number of elements in Flux

  • Before proceeding with hashing and saving, checked the number of elements in Flux to be exactly one, else an exception is thrown

  • For the current implementation, if there is error with target query, it will only be triggered after source has finished saving as target is only loaded after source is completed

  • Fix DatasetRecServiceTest tests for reconciliation of empty source and/or target to mock zero rows in result; it was previously mocking zero results which implied no sql query was configured

  • For stylistic consideration: For tests, use Kotlin extensions provided by Reactor flux.test().verifyComplete() instead of the Java default one StepVerifier.create(flux).verifyComplete()

@chadlwilson
Copy link
Collaborator Author

  • For stylistic consideration: For tests, use Kotlin extensions provided by Reactor flux.test().verifyComplete() instead of the Java default one StepVerifier.create(flux).verifyComplete()

@jiawen-tw Cool, didn't know that existed!

chadlwilson added a commit that referenced this issue May 25, 2022
This reverts commit d3b7913 - reverting for now, as this change appears to be breaking the batching.
@chadlwilson chadlwilson reopened this May 25, 2022
@chadlwilson
Copy link
Collaborator Author

Had to revert and re-open this as the earlier solution seemed to be interfering with the batching and breaking the reactive chain somehow. Current theory is that it has exposed that we are missing a unit test that ensures things work when there are batches in place.

If not strictly due to the batching, the switch to doOnNext -> single -> flatMapMany seems to cause issues.

Observed behaviour is that even with a single query, the reactive chain just dies/disappears with no feedback to the user. The HTTP request just blocks.

This can be tested manually using the big-table-postgres scenario.

chadlwilson added a commit that referenced this issue Jun 2, 2022
For some reason using `.single().flatMapMany()` seems to cause the Flux to get stuck sometimes, perhaps related to batching. This seems to work, although it's not really failing "fast" since it still has to execute the queries and it may have partially inserted some data for the original statement. Nonetheless, it does work sufficiently for now.
@chadlwilson chadlwilson self-assigned this Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers size:XS Extra small items
Projects
None yet
Development

No branches or pull requests

2 participants