solr functionalty was moved to sparate location #2196

ssuvorov-fls · 2023-01-30T06:26:06Z

No description provided.

chrisknoll · 2023-02-07T12:26:22Z

src/main/java/org/ohdsi/webapi/extcommon/ConfigurationInfo.java

@@ -1,4 +1,4 @@
-package org.ohdsi.webapi.info;
+package org.ohdsi.webapi.extcommon;


Don't move classes to new package in a minor release. 'info represents the namespace for classes that convey information about webapi to clients. extcommon sounds like some sort of 'shared extension', which is very different, and implies some sort of functionality that isn't described. Before making these types of architectural changes, they should be discussed and understood.

I've reverted moving classes to new package

anthonysena · 2023-02-07T14:08:26Z

@ssuvorov-fls - I've created #2201 to describe this issue based on discussion with @alex-odysseus and @chrisknoll. Please review that issue and add anything I may have missed. Thanks!

anthonysena · 2023-02-07T14:28:28Z

Linking to this PR: #1091 which is relevant to the changes that introduced SOLR functionality to the code base and also includes issues/discussions that are relevant to this set of changes too.

chrisknoll · 2023-02-07T15:40:05Z

src/main/java/org/ohdsi/webapi/vocabulary/DatabaseSearchProvider.java

    @Autowired
    VocabularyService vocabService;

    @Override
-    public boolean supports(VocabularySearchProviderType type) {
-        return Objects.equals(type, VocabularySearchProviderType.DATABASE);
+    public boolean supports(String vocabularyVersionKey) {


So, it appears that the original implementation provided an API to 'ask' the instance if it supported a particular Type. Ie: if you ask the SOLRSearchProvider if it supports VocabularySearchProviderType.DATABASE, it would return false, and if you asked the DatabaseProvider if it supports VocabularySearchProviderType.SOLR, it would return false. Now, supports seems to work off of a vocabularyVersionKey. This seems like a big divergance between the semantics of the supports() function. If we want something specific for vocabulary version support, shouldn't we have a function that indicates that we're asking something related to the vocabularyVersion?

chrisknoll · 2023-02-07T15:42:29Z

src/main/java/org/ohdsi/webapi/extcommon/vocabulary/SearchProvider.java

+
+public interface SearchProvider {
+    boolean supports(String vocabularyVersionKey);
+    int getPriority();


getPriority seems out of place here: priority is a something in the context of Source and SourceDaimon. Why would a SearchProvider have a priority?

ssuvorov-fls · 2023-02-08T11:40:16Z

@chrisknoll I've restored moved classes
next steps for 2.14:

Creating two repositories:
1.1. Repository for common classes which will be used in both webapi and solr modules
This repository will include the following classes:
org.ohdsi.webapi.vocabulary.Concept;
org.ohdsi.webapi.vocabulary.SearchProviderConfig;
org.ohdsi.webapi.vocabulary.SearchProvider;
org.ohdsi.webapi.info.ConfigurationInfo - this classes can be excluded but in this case we won't be able to get information about Solr config parameters
1.2. Repository for solr modules
org.ohdsi.webapi.vocabulary.solr.SolrSearchProvider;
org.ohdsi.webapi.vocabulary.solr.SolrSearchClient;
org.ohdsi.webapi.vocabulary.solr.SolrConfigurationInfo;
Adding profile to pom.xml in WebAPI

src/main/java/org/ohdsi/webapi/vocabulary/SearchProvider.java

chrisknoll · 2023-02-08T13:31:47Z

This repository will include the following classes:
org.ohdsi.webapi.vocabulary.Concept;

Perhaps the Concept class should be moved to StandardAnalysisAPI since that repo is designed to store analytic classes,

Note: I don't mean create an interface Concept (so that everyone who wants to handle Concept data they have to create their own ConceptImpl class...) The StandardAnalysisAPI can contain POJOs that represent data without behavior.

chrisknoll · 2023-02-13T14:54:37Z

My understanding of this PR is to prepare the codebase to be easier to 'extract' the SOLR implementation into a separate library such that WebAPI can be built without SOLR libraries if it isn't required.

I'm seeing at least two places that changes were made but I do not see how it applies to this function, and wanted to get clarity:

The supports() method of SearchProvider changed from VocabulyarsearchProviderType to String (a vocabularyVersionKey). This seems to be adding a new feature where you can ask the search provider if it supports a vocabulary version? Was it doing something like this before? Is the change that there will be multiple SOLR cores installed with different vocabulary versions and getSearchProvider() will find the cores for that given vocabulary version? What's strange to me about this is that now people may have to pay attention more to vocabulary versions of their cores, and I don't think they had to do that in the past: you just put a core for SOLR to use and whatever version is there it would query.
Related to the above, there's a hew function of 'getPriority(). Based on the above, it would seem that priority would be used to order the cores by priority, but looking at the implementation of getPriority(), it returns a constant SOLR_PRIORITY. So, is this notion of a 'priority' something new to this PR?

Additional thoughts on externalizing the search providers:

We should probably say away from using WebAPI-specific terms from the provider and interface, specifically anything related to Source. Source is something that exists only in the WebAPI domain, and to 'leak' Source-specific contexts will lead to unnecessary coupling between WebAPI and these external services.

ssuvorov-fls · 2023-02-14T09:09:35Z

It is not functionality. It is taken from VocabularySearchServiceImpl.getSearchProvider. The Solr search can't be used when there is no index for version key
GetPriority is used to get appropriate search provider. Solr has higher priority and will be used instead of vocabulry search when Solr module is enabled

…oring

chrisknoll · 2023-02-21T15:26:38Z

@ssuvorov-fls I think I found an issue post-merge: When I start up the app but use the default solr settings of <solr.endpoint></solr.endpoint>, it reports an exception during startup:

org.apache.solr.client.solrj.SolrServerException: IOException occurred when talking to server at:

Looking closer at it, I'm not sure where we check in the new code the value of solr.endpoint to disable the call to SOLR.

Can you try this on your local env?

ssuvorov-fls · 2023-02-21T16:15:27Z

I removed it from code as I think that when we include solr starter then we need to get exception in case of lack of configuration.
But it seems that you are right and we need to check availability now
I'll create another pr with these changes

…on to a separate library OHDSI/WebAPI#2201 Based on the idea OHDSI/WebAPI#2196 Referencing the latest Standardized Analysis Utils and Circe 1.5.0 release preparation

solr functionalty was moved to sparate location

b360cbb

ssuvorov-fls requested a review from anton-abushkevich January 30, 2023 10:14

ssuvorov-fls mentioned this pull request Jan 30, 2023

Add concept sets search #2177

Closed

anton-abushkevich requested a review from chrisknoll February 7, 2023 10:17

chrisknoll requested changes Feb 7, 2023

View reviewed changes

anthonysena added this to the v2.13 milestone Feb 7, 2023

anthonysena mentioned this pull request Feb 7, 2023

Isolate and externalize SOLR functionality #2201

Closed

anthonysena linked an issue Feb 7, 2023 that may be closed by this pull request

Isolate and externalize SOLR functionality #2201

Closed

anthonysena removed this from the v2.13 milestone Feb 7, 2023

chrisknoll reviewed Feb 7, 2023

View reviewed changes

fixed after review

0812b66

fixed after review

9f17190

chrisknoll reviewed Feb 8, 2023

View reviewed changes

src/main/java/org/ohdsi/webapi/vocabulary/SearchProvider.java Outdated Show resolved Hide resolved

Merge remote-tracking branch 'remotes/origin/master' into solr_refact…

0583a02

…oring

chrisknoll approved these changes Feb 16, 2023

View reviewed changes

chrisknoll merged commit f17a1b7 into master Feb 16, 2023

delete-merged-branch bot deleted the solr_refactoring branch February 16, 2023 01:01

This was referenced Sep 11, 2023

Vocabulary search common classes OHDSI/StandardizedAnalysisAPI#56

Merged

Finalizing externalizing Vocabulary Search Apache Solr implementation #2308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

solr functionalty was moved to sparate location #2196

solr functionalty was moved to sparate location #2196

ssuvorov-fls commented Jan 30, 2023

chrisknoll Feb 7, 2023

ssuvorov-fls Feb 8, 2023

anthonysena commented Feb 7, 2023 •

edited

anthonysena commented Feb 7, 2023

chrisknoll Feb 7, 2023

chrisknoll Feb 7, 2023

ssuvorov-fls commented Feb 8, 2023

chrisknoll commented Feb 8, 2023

chrisknoll commented Feb 13, 2023

ssuvorov-fls commented Feb 14, 2023

chrisknoll commented Feb 21, 2023

ssuvorov-fls commented Feb 21, 2023

		@@ -1,4 +1,4 @@
		package org.ohdsi.webapi.info;
		package org.ohdsi.webapi.extcommon;

solr functionalty was moved to sparate location #2196

solr functionalty was moved to sparate location #2196

Conversation

ssuvorov-fls commented Jan 30, 2023

chrisknoll Feb 7, 2023

Choose a reason for hiding this comment

ssuvorov-fls Feb 8, 2023

Choose a reason for hiding this comment

anthonysena commented Feb 7, 2023 • edited

anthonysena commented Feb 7, 2023

chrisknoll Feb 7, 2023

Choose a reason for hiding this comment

chrisknoll Feb 7, 2023

Choose a reason for hiding this comment

ssuvorov-fls commented Feb 8, 2023

chrisknoll commented Feb 8, 2023

chrisknoll commented Feb 13, 2023

ssuvorov-fls commented Feb 14, 2023

chrisknoll commented Feb 21, 2023

ssuvorov-fls commented Feb 21, 2023

anthonysena commented Feb 7, 2023 •

edited