SO-2127 snomed rf2 importer issues #122

cmark · 2016-12-12T09:35:58Z

SNOMED CT RF2 importer enhancements, bugfixes:

Batch processing (execute batch index queries instead of single item queries to improve performance)
Reduce memory requirement of FULL RF2 imports (SG dataset successfully completes with 6-8 GB)
FULL import allows unpublished components to be imported
Skip cycle detection
Embedded SCT ID service is now properly execute bulk operations (using properly configured index queries to load all available document with a single query)
Component caching now supports bulk loading/lookup of SCT components
Run purge/optimize operations after versioning to remove garbage from index
Load partial documents where possible

Clear unnecessary allocated data when after validator.doValidate(). Validate Stated Relationships file instead of validating Relationships file twice. Report errors to a files instead of console. Report taxonomy errors by effective time. Allow unpublished components to be imported from FULL RF2 files.

...instead of Strings Add writeTo(Writer) method to SnomedValidationDefect.

Bulk ID operations will load documents in bulk instead of one by one before executing the operation.

...instead of referencing variable from anonym class. This will ensure that only relevant information from the RF2 rows will be kept in memory and not the entire row.

Component importers will read RF2 rows until they reach the specified threshold (50000 atm) and then start processing them instead of processing the rows one by one. This way the importer will execute way more less index lookups than before (single index query per 50000 rows to get the current revision).

…ed-rf2-importer-issues

...for the last imported RF2 effective time

Fetch Description objects by storageKey and attach new members to lang members list directly instead of hacking with low-level revision objects.

apeteri

I'm happy with the changes!

Please take a look at the review comments below; there doesn't seem to be any big stuff to move around. Some of them are already outdated because I reviewed commits in chronological order. 😄

apeteri · 2017-01-03T10:35:17Z

...snomed.importer.rf2/src/com/b2international/snowowl/snomed/importer/rf2/util/ImportUtil.java

+					try {
+						for (SnomedValidationDefect defect : defects) {
+							final String filePath = defect.getFileName();
+							final String defectsFile = String.format("d:/%s_%s_defects.txt", configuration.getArchiveFile().getName(), filePath);


Hard-coded file location above 👓

apeteri · 2017-01-03T10:38:34Z

...snomed.importer.rf2/src/com/b2international/snowowl/snomed/importer/rf2/util/ImportUtil.java

+							}
+							for (String lineDefect : defect.getDefects()) {
+								defectWriters.get(defectsFile).write(defect.getDefectType().name() + "\t" + lineDefect);
+								defectWriters.get(defectsFile).write(lineSeparator);


BufferedWriter exposes a newLine method, which uses the line.separator system property as well, so lineSeparator could be removed if it's not used elsewhere.

apeteri · 2017-01-03T10:41:05Z

...d.importer.rf2/src/com/b2international/snowowl/snomed/importer/rf2/util/Rf2FileModifier.java

@@ -38,6 +38,8 @@
 */
 public class Rf2FileModifier {

+	private static final Splitter ON = Splitter.on('\t');


Please rename this constant so that it indicates that it's a Splitter.

apeteri · 2017-01-03T10:47:09Z

...f2/src/com/b2international/snowowl/snomed/importer/rf2/validation/SnomedRefSetValidator.java

@@ -160,6 +160,7 @@ protected void doValidate(String effectiveTime, IProgressMonitor monitor) {
 		addDefect(DefectType.REFSET_MEMBER_COMPONENT_NOT_EXIST, referencedComponentNotExist);
 		uuidInvalid.clear();
 		referencedComponentNotExist.clear();
+		memberDataByUuid.clear();


uuidNotUnique is not cleared here. Do all fields need to be cleared in all validators after checking an effectiveTime layer? If not, please revisit doValidate methods and check if the appropriate fields are cleared.

It depends on the field whether it needs to be cleared or not after doValidate(). uuidNotUnique seems to be used to report ID uniqueness issues independent of the effective time.

apeteri · 2017-01-03T11:02:24Z

.../src/com/b2international/snowowl/snomed/importer/rf2/validation/SnomedTaxonomyValidator.java

-				final Collection<String> defects = newHashSet();
-				final Collection<String> conceptIdsToInactivate = newHashSet();
-
+				final Collection<TaxonomyDefect> defects = newArrayListWithExpectedSize(invalidRelationships.size());
 				for (final String effectiveTime : invalidRelationships.keySet()) {


Note that ArrayListMultimap's key set might not preserve the ordering used in processTaxonomy. (Not sure if we are relying on it here.)

apeteri · 2017-01-03T14:03:52Z

...2/src/com/b2international/snowowl/snomed/importer/rf2/terminology/SnomedConceptImporter.java

-			editedConcept.setEffectiveTime(currentRow.getEffectiveTime());
-			editedConcept.setReleased(true);
+	protected void applyRow(Concept component, ConceptRow row, Collection<Concept> componentsToAttach) {
+		component.setId(row.getId());


Just as in reference set importers, core component importers could also avoid setting the SCTID twice.

apeteri · 2017-01-03T14:06:42Z

...c/com/b2international/snowowl/snomed/importer/rf2/terminology/SnomedDescriptionImporter.java

-
-		return description;
+	protected void attach(Collection<Description> componentsToAttach) {
+		// nothing to do


A thought: it might be quicker to store the concept IDs instead of resolving them in applyRow, retrieve the actual concepts with a single call here, then use concept.getDescriptions().add(description) on the appropriate concept. But we'd have to test this somehow.

That was my original intention with the attach method, use it everywhere, but I found the current impl. good enough (in terms of performance) so I did not change these. (takes ~1 sec to process 50.000 descriptions/relationships at the moment)

apeteri · 2017-01-03T14:09:37Z

.../com/b2international/snowowl/snomed/importer/rf2/terminology/SnomedRelationshipImporter.java

+		component.setActive(row.isActive());
+		component.setModule(getConceptSafe(row.getModuleId(), SnomedRf2Headers.FIELD_MODULE_ID, row.getId()));
+
+		if (component.getSource() == null || !component.getSource().getId().equals(row.getSourceId())) {


Do we have to accept import files where the source ID gets changed? Is the SCT change processor prepared to properly index these changes?

Hopefully, no. 😄

apeteri · 2017-01-03T14:10:16Z

.../com/b2international/snowowl/snomed/importer/rf2/terminology/SnomedRelationshipImporter.java

-
-		return relationship;
+	protected void attach(Collection<Relationship> componentsToAttach) {
+		// nothing to do, we already registered the relationships to the source concepts


(Same as with descriptions.)

apeteri · 2017-01-03T14:16:41Z

...er.rf2/src/com/b2international/snowowl/snomed/importer/rf2/model/AbstractSnomedImporter.java

@@ -406,8 +402,7 @@ private ImportAction checkHeaders(final String[] expectedHeader, final String[]
 	}

 	private ImportAction handlePreImportException(final SuperCSVException e) {
-		final String reason = null != e.getMessage() ? " Reason: '" + e.getMessage() + "'" : "";
-		log("Exception caught while reading release file. Continuing with next row." + reason);
+		log("Exception caught while reading release file. Continuing with next row. {}", e.getMessage());


If the vararg Objects land in an SLF4J Logger, we could propagate exceptions as an additional parameter now, to get a stack trace along with the exception message in the log output. There are other opportunities to do so below.

apeteri · 2017-01-06T10:03:40Z

...er.rf2/src/com/b2international/snowowl/snomed/importer/rf2/model/AbstractSnomedImporter.java

@@ -633,13 +634,13 @@ protected void preCommit(final InternalCDOTransaction transaction) throws Snowow
 	}

 	private ImportAction checkCommitException(final SnowowlServiceException e) {
-		log("SNOMED CT import failed. Caught exception while import, aborting. {}", e.getMessage());
+		log("SNOMED CT import failed. Caught exception while import, aborting. {}", e);


The placeholder should be removed from this format string.

apeteri · 2017-01-06T10:03:50Z

...er.rf2/src/com/b2international/snowowl/snomed/importer/rf2/model/AbstractSnomedImporter.java

 		importContext.getLogger().warn("Caught exception while import, aborting.", e);
 		return ImportAction.BREAK;
 	}

 	private ImportAction checkCommitException(final CommitException e) {
-		log("SNOMED CT import failed. Caught exception while import, aborting. {}", e.getMessage());
+		log("SNOMED CT import failed. Caught exception while import, aborting. {}", e);


(also here)

apeteri · 2017-01-06T10:06:03Z

Good to merge otherwise. ✔️

cmark · 2017-01-06T10:11:36Z

Thank you!

cmark added 21 commits November 24, 2016 11:11

SO-1782: store taxonomy defect information in data objects...

3fe1824

...instead of Strings Add writeTo(Writer) method to SnomedValidationDefect.

SO-2127: fix NPE when getting datatype for cd member rows

0a46423

SO-2127: skip cycle detection during import

c44cee9

SO-2127: improve performance of in-memory ID service

df2aa25

Bulk ID operations will load documents in bulk instead of one by one before executing the operation.

SO-2127: extract relevant taxonomy information for nodes and edges...

ce44373

...instead of referencing variable from anonym class. This will ensure that only relevant information from the RF2 rows will be kept in memory and not the entire row.

SO-2127: support bulk methods in import time component lookup impls

fb31a71

SO-2127: disable error reporting functionality

9613dd5

SO-2127: batch RF2 processing improvements

923c222

SO-2127: get temp CDOResource outside the for loop (lang.refset import)

d38dcf8

SO-2127: remove unnecessary setId calls from member importers

0f9b4a1

Merge remote-tracking branch 'origin/develop' into issue/SO-2127-snom…

6de7228

…ed-rf2-importer-issues

SO-2127: fix concrete domain member index entry building

51b9bc3

SO-2127: use log message arguments instead of string concatenation

0331d77

SO-2127: run index purge/optimize before creating version...

c59ecd7

...for the last imported RF2 effective time

SO-2127: remove commit notification from notification queue

9b96893

SO-2127: enqueue commit notifications when enabled in tx context

a01efba

SO-2127: remove temporary resource form lang. refset importer

59e33b5

Fetch Description objects by storageKey and attach new members to lang members list directly instead of hacking with low-level revision objects.

SO-2127: change importer logger names to snomed.importer.rf2

35eefec

SO-2127: reduce document load times during import

f7a3f56

cmark requested review from apeteri and nagyo December 12, 2016 09:35

apeteri approved these changes Jan 3, 2017

View reviewed changes

cmark added 6 commits January 4, 2017 17:05

SO-2127: rename constant from ON to TAB_SPLITTER

8934bf7

SO-2127: fix comments in import time lookup services

07770db

SO-2127: include stack trace in log messages

d2f5bce

SO-2127: use rows.clear instead of recreating List instance

d3e48b6

SO-2127: simplify processing of rows

4105458

SO-2127: rename component creation template methods

e3e04ed

cmark added 3 commits January 5, 2017 17:24

SO-2127: generate available identifiers in bulk load method for symmetry

efecf53

SO-2127: avoid setting core component identifier twice

1b80174

SO-2127: rename field extended to hasMapTargetDescription

b780da1

apeteri reviewed Jan 6, 2017

View reviewed changes

cmark merged commit ca6c944 into develop Jan 6, 2017

cmark deleted the issue/SO-2127-snomed-rf2-importer-issues branch June 19, 2017 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SO-2127 snomed rf2 importer issues #122

SO-2127 snomed rf2 importer issues #122

cmark commented Dec 12, 2016

apeteri left a comment

apeteri Jan 3, 2017

apeteri Jan 3, 2017

apeteri Jan 3, 2017

apeteri Jan 3, 2017

cmark Jan 4, 2017 •

edited

apeteri Jan 3, 2017

apeteri Jan 3, 2017

apeteri Jan 3, 2017

cmark Jan 5, 2017

apeteri Jan 3, 2017

cmark Jan 5, 2017

apeteri Jan 3, 2017

apeteri Jan 3, 2017

apeteri Jan 6, 2017

apeteri Jan 6, 2017

apeteri commented Jan 6, 2017

cmark commented Jan 6, 2017

SO-2127 snomed rf2 importer issues #122

SO-2127 snomed rf2 importer issues #122

Conversation

cmark commented Dec 12, 2016

apeteri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmark Jan 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apeteri commented Jan 6, 2017

cmark commented Jan 6, 2017

cmark Jan 4, 2017 •

edited