Add concept sets search #2177
Add concept sets search #2177
Conversation
…bAPI into add_concept_sets_search
There was a problem hiding this comment.
This is my first pass:
This PR is extremely large, touching 122 files,, and ~9k lines to review. The initial issue (OHDSI/Atlas#2803) is very light on functional details about what the functionality is doing.
We had initial meetings about what the functionality was, and my understanding was at the time we would limit this type of search to find concepts by what was specified in the literal concept set, and not what may be referenced in an included concepts or mapped (source) concepts. We also didn't include solr in the original discussions.
What is in this PR is much larger, more complex, and potentially extremely resource heavy on the database servers in order to perform re-indexing. I have concerns about how this is actually going to perform, but at this point I have only given this the high-level review and have not executed the implementation locally. The challenges to this is 1) a reasonable environment to do proper workload evaluation, and 2) local dev environment does not have SOLR enabled so I will need to enabled that locally in order to use this function.
I would ask that the initial issue (OHDSI/Atlas#2803) be amended to include more functional details about how this feature works:
- How is it invoked
- How much additional resources does supporting this feature put on a system?
- There is references to index refresh, but who does it? Is it done every time an asset is saved, or some administrator need to kick off the job?
- How is it disabled?
That's a good start....more to come as this function is reviewed more...
| @@ -1,16 +1,14 @@ | |||
| package org.ohdsi.webapi.info; | |||
| package org.ohdsi.webapi.extcommon; | |||
There was a problem hiding this comment.
Do not rename packages as part of a minor release.
| @@ -0,0 +1,14 @@ | |||
| package org.ohdsi.webapi.extcommon.vocabulary; | |||
There was a problem hiding this comment.
The package name 'extcommon' is confusing.
| @Autowired | ||
| VocabularyService vocabService; | ||
|
|
||
| @Override | ||
| public boolean supports(VocabularySearchProviderType type) { | ||
| return Objects.equals(type, VocabularySearchProviderType.DATABASE); | ||
| public boolean supports(String vocabularyVersionKey) { |
There was a problem hiding this comment.
2 things here: why was public method sig changed from enum to string, and why aren't we using enums?
Where does the value of the vocabularyVersionKey come from?
| }, | ||
| "changeSourcePriorities": "Change source priorities in:", | ||
| "confirms": { | ||
| "clearServerCache": "Are you sure you want to clear the server cache?" | ||
| "clearServerCache": "Are you sure you want to clear the server cache?", | ||
| "reindexSource": "Reindexing may take a long time. It depends on amount and complexity of concept sets" |
There was a problem hiding this comment.
Is the process of reindexing (ie: what it does and who would invoke this function) described anywhere?
|
some changes already exist in #2196 |
…arch # Conflicts: # src/main/resources/i18n/messages_en.json # src/main/resources/i18n/messages_ko.json # src/main/resources/i18n/messages_ru.json # src/main/resources/i18n/messages_zh.json
|
Moving to v2.14 per OHDSI/Atlas#2803 (comment) |
|
Closing in favor of #2315 |
Backend for OHDSI/Atlas#2803
Solr core configuration for CS Search was added to
src/main/resources/solr/concept-sets.Solr core config for Concepts Search was moved from
src/main/resources/solrtosrc/main/resources/solr/vocabulary.