Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TASK-6565 Ensure the CellBase DB adaptors are updated to reflect the latest data releases #700

Open
wants to merge 14 commits into
base: release-6.x.x
Choose a base branch
from

Conversation

jtarraga
Copy link
Member

@jtarraga jtarraga commented Jul 19, 2024

juanfeSanahuja and others added 7 commits July 5, 2024 09:25
On branch TASK-6565
Changes to be committed:
	new file:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingleton.java
	new file:   cellbase-lib/src/test/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingletonTest.java
…tants, #TASK-6565

On branch TASK-6565
Changes to be committed:
	modified:   cellbase-app/src/main/java/org/opencb/cellbase/app/cli/admin/executors/LoadCommandExecutor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/EtlCommons.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/CellBaseDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/ClinicalMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/GeneMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/GenomeMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/MetaMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/MissenseVariationFunctionalScoreMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/OntologyMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/PharmacogenomicsMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/ProteinMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/PublicationMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/RegulationMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/RepeatsMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/SnpMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/SpliceScoreMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/TranscriptMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/VariantMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/XRefMongoDBAdaptor.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/managers/CellBaseManagerFactory.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/managers/DataReleaseManager.java
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/variant/annotation/VariantAnnotationCalculator.java
On branch TASK-6565
Changes to be committed:
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingleton.java
	modified:   cellbase-server/src/main/java/org/opencb/cellbase/server/rest/GenericRestWSServer.java
On branch TASK-6565
Changes to be committed:
	modified:   cellbase-server/src/main/java/org/opencb/cellbase/server/rest/GenericRestWSServer.java
On branch TASK-6565
Changes to be committed:
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingleton.java
On branch TASK-6565
Changes to be committed:
	modified:   cellbase-lib/src/main/java/org/opencb/cellbase/lib/impl/core/DataReleaseSingleton.java
@jtarraga jtarraga requested review from imedina and j-coll July 19, 2024 11:43
@jtarraga jtarraga changed the title Ensure the CellBase DB adaptors are updated to reflect the latest data releases TASK-6565 Ensure the CellBase DB adaptors are updated to reflect the latest data releases Jul 19, 2024
try {
handleDocumentChange(changeStreamDocument);
} catch (CellBaseException e) {
LOGGER.warn("Exception from handle document change function: {}", e.getStackTrace());
Copy link
Member

Choose a reason for hiding this comment

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

Improper exception reporting. The whole exception should be the last element of the logger.

LOGGER.warn("Exception from handle document change function", e);

Also, might want to add more details on the collection watcher that failed.

Copy link
Member

Choose a reason for hiding this comment

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

And what if the whole collection.watch()... fails? The whole code of the thread should be within a pseudo infinite loop, catching any Exception, and check for thread interruptions within the loop

while (!Thread.currentThread().isInterrupted()) {
  try {
     MongoCursor iterator = collection.watch().fullDocument(FullDocument.UPDATE_LOOKUP).iterator();
     while (!Thread.currentThread().isInterrupted() && iterator.hasNext()) {
        // ...
     }
  } catch (Exception e) {
    LOGGER.warn("...", e);
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Improper exception reporting. The whole exception should be the last element of the logger.

LOGGER.warn("Exception from handle document change function", e);

Also, might want to add more details on the collection watcher that failed.

maybe it's better to display the strack trace, e.g.:
LOGGER.warn("Exception from handle document change function: {}", Arrays.toString(e.getStackTrace()));

Copy link
Member

Choose a reason for hiding this comment

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

Having the exception as the last argument in a logger will print the whole stacktrace

        try {
            throw new RuntimeException("myException");
        } catch (Exception e) {
            logger.warn("This is an exception", e);
        }
2024-08-08 12:03:27 [main] WARN  VariantQueryExecutorTest:59 - This is an exception
java.lang.RuntimeException: myException
	at org.opencb.opencga.storage.core.variant.query.executors.VariantQueryExecutorTest.setUp(VariantQueryExecutorTest.java:57) ~[test-classes/:?]
	at org.opencb.opencga.storage.hadoop.variant.query.executors.HadoopVariantQueryExecutorTest.setUp(HadoopVariantQueryExecutorTest.java:25) ~[test-classes/:?]
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_161]
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:1.8.0_161]
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_161]
	at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_161]
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59) ~[junit-4.13.2.jar:4.13.2]
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) ~[junit-4.13.2.jar:4.13.2]
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56) ~[junit-4.13.2.jar:4.13.2]
	at org.junit.internal.runners.statements.RunBefores.invokeMethod(RunBefores.java:33) ~[junit-4.13.2.jar:4.13.2]
.....

LOGGER.warn("Exception from handle document change function: {}", e.getStackTrace());
}
});
}).start();
Copy link
Member

Choose a reason for hiding this comment

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

All these threads should be within a single "ExecutionService". Then, as this is a singleton class, and can't be closed easily, a ShutdownHook should terminate the ExecutionService

    Runtime.getRuntime().addShutdownHook(new Thread(() -> {
        threadPool.shutdownNow();
    });

@jtarraga jtarraga requested a review from j-coll August 8, 2024 10:27
DatabaseInfo dbInfo = getDatabaseInfo(dbname);

// Lock and load data if necessary
dbInfo.getRwLock().writeLock().lock();
Copy link
Member

Choose a reason for hiding this comment

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

This would be an unneeded lock if dbInfo.getCacheData().containsKey(release) and dbInfo.getCacheData().get(release).containsKey(data). Given that this method will be called a lot of times, might be worth improving the logic?

private String dbName;
private String species;
private String assembly;
private ReentrantReadWriteLock rwLock;
Copy link
Member

Choose a reason for hiding this comment

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

Do you actually need a ReentrantReadWriteLock? Why not using a ReentrantLock?

@@ -85,13 +79,31 @@ private DataReleaseSingleton(CellBaseManagerFactory managerFactory) throws CellB
.getCollectionName());
// Set up the change stream for the collection
new Thread(() -> {
collection.watch().fullDocument(FullDocument.UPDATE_LOOKUP).forEach(changeStreamDocument -> {
while (true) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd still have here a !Thread.currentThread().isInterrupted()

@jtarraga jtarraga changed the base branch from release-5.8.x to release-6.2.x August 13, 2024 10:07
@jtarraga jtarraga requested a review from j-coll August 13, 2024 12:09
j-coll
j-coll previously approved these changes Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants