Search SDK Sync Stack Integration #32286
Conversation
ghost
added
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
g2vinay
requested review from
alzimmermsft,
jairmyree,
srnagar,
anuchandy,
lmolkova,
vcolin7,
mssfang,
JonathanGiles and
billwert
as code owners
g2vinay
changed the title
| @@ -80,7 +80,7 @@ public PagedIterableBase(PagedFluxBase<T, P> pagedFluxBase) { | |||
| * | |||
| * @param provider the Page Retrieval Provider | |||
| */ | |||
| PagedIterableBase(Supplier<PageRetrieverSync<String, P>> provider) { | |||
There was a problem hiding this comment.
yes, it is needed to unblock the custom Paged Iterable implementations in Search SDK.
g2vinay
force-pushed
the
kv-search-sync-stack
branch
from
95c43e7 to
d502c8a
Compare
g2vinay
force-pushed
the
kv-search-sync-stack
branch
from
a48a6b3 to
05218b9
Compare
...earch/azure-search-documents/src/main/java/com/azure/search/documents/SearchAsyncClient.java
Show resolved
Hide resolved
| return new SimpleResponse<>(response, Utility.convertValue(response.getValue(), modelClass)); | ||
| } catch (IOException ex) { | ||
| throw LOGGER.logExceptionAsError( | ||
| new RuntimeException("Failed to deserialize document.", ex)); |
There was a problem hiding this comment.
I'd use UncheckedIOException here instead of RuntimeException. Also how does this handle with the wrapping utility method?
There was a problem hiding this comment.
This method is unique with exception handling.
Localized exception handling for this method instead of using utility flow.
| Boolean includeTotalCountInternal = null; | ||
| if (searchOptions != null) { | ||
| includeTotalCountInternal = searchOptions.isTotalCountIncluded(); | ||
| } | ||
| Boolean includeTotalCount = includeTotalCountInternal; |
There was a problem hiding this comment.
TODO for myself, file an issue against the autorest.java repo to clean up this pattern, this can be replaced with a terser and more readable pattern.
Boolean includeTotalCount = (searchOptions != null) ? searchOptions.isTotalCountIncluded() : null;
...e-search-documents/src/main/java/com/azure/search/documents/implementation/util/Utility.java
Show resolved
Hide resolved
...e-search-documents/src/main/java/com/azure/search/documents/implementation/util/Utility.java
Outdated
Show resolved
Hide resolved
| @@ -111,7 +111,7 @@ This swagger is ready for C# and Java. | |||
| output-folder: ../ | |||
| java: true | |||
| use: '@autorest/java@4.1.2' | |||
There was a problem hiding this comment.
Does the version of Autorest need to be updated here?
There was a problem hiding this comment.
not sure, if @srnagar has released the Autorest version with sync stack support.
if its available, we can update it.
| createInstance(modelClass)); | ||
| return new SimpleResponse<>(response, doc); | ||
| } catch (SearchErrorException ex) { | ||
| throw LOGGER.logExceptionAsError(DocumentResponseConversions.mapSearchErrorException(ex)); |
There was a problem hiding this comment.
@alzimmermsft, should we log this exception here ?
is this already logged somewhere in core pipeline flow ?
There was a problem hiding this comment.
It's likely already logged somewhere else in the pipeline but I'm not 100% certain where this would happen. I think we can punt on this for a holistic discussion on logging and prevention of duplicative error logging.
Integrates Sync Stack in Azure Search Documents SDK
Note: Test Coverage needs to be added for async clients for this PR to be merged. #32360
Sync Stack Coverage
Onboards these key components in Azure Search SDK to Sync Stack.
Public APIs Added
Azure Core
Azure Search SDK
Search Paged Iterable
Applies to Suugest and AutoCompleteIterable.