Skip to content

Expose merge bug as test was swallowing it - #142

Merged
sam-herman merged 2 commits into
opensearch-project:mainfrom
tjake:merge-bug-test
Aug 28, 2025
Merged

Expose merge bug as test was swallowing it#142
sam-herman merged 2 commits into
opensearch-project:mainfrom
tjake:merge-bug-test

Conversation

@tjake

@tjake tjake commented Aug 28, 2025

Copy link
Copy Markdown
Collaborator

This pull request improves resource management and enhances the robustness and logging of concurrent kNN search tests in the JVectorReader and KNNJVectorTests classes.

It was silently failing!

Test Robustness and Logging Improvements:

  • Improved exception handling in the concurrent search test by catching Throwable instead of Exception, using compareAndSet for atomic failure flag updates, and logging encountered exceptions.
  • Added per-thread logging of the number of queries executed for better visibility during test runs.
  • Added an assertion to verify the total number of queries executed matches expectations, and updated the success log message for clarity.

Code Clarity:

  • Refactored the query counter in the concurrent search test for clearer tracking of queries executed per thread.### Description
    [Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

}
} finally {
latch.countDown();
log.warn("Ran " + i + " queries");

@sam-herman sam-herman Aug 28, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: log.warn("Ran {} queries", i)

log message

Signed-off-by: sam-herman <97131656+sam-herman@users.noreply.github.com>
@sam-herman
sam-herman merged commit 5501720 into opensearch-project:main Aug 28, 2025
6 of 38 checks passed
opensearch-trigger-bot Bot pushed a commit that referenced this pull request Aug 28, 2025
* Expose merge bug as test was swallowing it

* Update KNNJVectorTests.java

log message

Signed-off-by: sam-herman <97131656+sam-herman@users.noreply.github.com>

---------

Signed-off-by: sam-herman <97131656+sam-herman@users.noreply.github.com>
Co-authored-by: sam-herman <97131656+sam-herman@users.noreply.github.com>
(cherry picked from commit 5501720)
sam-herman added a commit that referenced this pull request Aug 28, 2025
* Expose merge bug as test was swallowing it

* Update KNNJVectorTests.java

log message



---------



(cherry picked from commit 5501720)

Signed-off-by: sam-herman <97131656+sam-herman@users.noreply.github.com>
Co-authored-by: Jake Luciani <jake@datastax.com>
Co-authored-by: sam-herman <97131656+sam-herman@users.noreply.github.com>
@sam-herman sam-herman mentioned this pull request Aug 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants