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.

Sign up for GitHub

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

Jump to bottom

issue #1708 store compartment search parameters for faster compartment search queries #1740

Merged
merged 10 commits into from
Nov 23, 2020
Merged

issue #1708 store compartment search parameters for faster compartment search queries #1740

merged 10 commits into from
Nov 23, 2020

Conversation

punktilious
Copy link
Collaborator

@punktilious punktilious commented Nov 20, 2020 •
edited
Loading

This PR now also includes a fix for #1742 to correctly extract and store all search parameters during a reindex operation.

@punktilious
issue #1708 add custom compartment reference params for faster compar…
d862936 
…tment searches

Signed-off-by: Robin Arnold <robin.arnold23@ibm.com>
@punktilious
Merge remote-tracking branch 'origin/master' into robin-proto
398bbdc
@punktilious
issue #1708 use ibm-internal stored parameters for compartment searches
2d89c8a 
Signed-off-by: Robin Arnold <robin.arnold23@ibm.com>
@punktilious
issue #1708 merge with master and handle NPE when search param is fil…
d8aca7d 
…tered out

Signed-off-by: Robin Arnold <robin.arnold23@ibm.com>
prb112
prb112 reviewed Nov 21, 2020
View reviewed changes
...ence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/impl/ParameterVisitorBatchDAO.java Outdated
if (refValue.getType() == ReferenceType.DISPLAY_ONLY || refValue.getType() == ReferenceType.INVALID) {
// protect against code regression. Invalid/improper references should be
// filtered out already.
logger.warning("Invalid reference parameter type: " + resourceType + "." + rpv.getName() + " type=" + refValue.getType().name());
Copy link
Contributor

Choose a reason for hiding this comment

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

see subsequent comment...

otherwise delimit the log message with a single quote.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an internal programming error, so I don't think the details need to be propagated to the outside world, but we still need to log the information to help debug the problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

hum... then this should be logger.fine

@punktilious
issue #1708 addressed code review comments and fixed reindexing as it…
77fd427 
… is required for storing compartment references

Signed-off-by: Robin Arnold <robin.arnold23@ibm.com>
@punktilious
issue #1708 updated docs per review comments
d14f775 
Signed-off-by: Robin Arnold <robin.arnold23@ibm.com>
This was linked to issues Nov 22, 2020
ParameterVisitorBatchDAO uses fixed string as a server base #1635
Closed
Remove _ from reindex custom operation parameter names #1641
Closed
Suboptimal compartment search execution plan on Postgres #1708
Closed
reindex operation does not compute all search parameters #1742
Closed
@punktilious
issue #1708 updated copyright header
cca3d4a 
Signed-off-by: Robin Arnold <robin.arnold23@ibm.com>
@punktilious
issue #1708 removed excess debug logging
74a6142 
Signed-off-by: Robin Arnold <robin.arnold23@ibm.com>
prb112
prb112 reviewed Nov 23, 2020
View reviewed changes
fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/ReindexResourceDAO.java Outdated
+ "OFFSET ? ROWS FETCH FIRST 1 ROWS ONLY "
;

String select;
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps declare as final and make the options constants within the class?

prb112
prb112 reviewed Nov 23, 2020
View reviewed changes
...rsistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java
@@ -1733,21 +1781,21 @@ public boolean isReindexSupported() {
}

@Override
public int reindex(FHIRPersistenceContext context, OperationOutcome.Builder operationOutcomeResult, java.time.Instant tstamp)
public int reindex(FHIRPersistenceContext context, OperationOutcome.Builder operationOutcomeResult, java.time.Instant tstamp, String resourceLogicalId)
Copy link
Contributor

Choose a reason for hiding this comment

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

so we can execute a single reindex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I added this to debug the issue Karen/Lee identified. It's extremely handy!

prb112
prb112 reviewed Nov 23, 2020
View reviewed changes
...rsistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java Outdated
} else {
// Reasonable to assume that this resource was deleted because we can't read it
final String diag = "Failed to read resource: " + rir.getResourceType() + "/" + rir.getLogicalId();
operationOutcomeResult.issue(Issue.builder().code(IssueType.NOT_FOUND).severity(IssueSeverity.WARNING).diagnostics(com.ibm.fhir.model.type.String.of(diag)).build());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where I like to static import the string method.

import static com.ibm.fhir.model.type.String.string;

string(diag)

makes it short and sweet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this should be updated. Fixed

prb112
prb112 reviewed Nov 23, 2020
View reviewed changes
...-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/postgresql/PostgresReindexResourceDAO.java Outdated
try (PreparedStatement stmt = connection.prepareStatement(UPDATE)) {
stmt.setTimestamp(1, Timestamp.from(reindexTstamp));
stmt.setTimestamp(2, Timestamp.from(reindexTstamp));
String update;
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this should be final or treated as a constant.

prb112
prb112 reviewed Nov 23, 2020
View reviewed changes
...on/fhir-operation-reindex/src/main/java/com/ibm/fhir/operation/reindex/ReindexOperation.java
@@ -92,12 +94,14 @@ protected Parameters doInvoke(FHIROperationContext operationContext, Class<? ext
// assume full ISO format
tstamp = Instant.parse(val);
}

} else if (PARAM_RESOURCE_COUNT.equals(parameter.getName().getValue())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably have a defense against say 10000000 resources get reindexed. What's a reasonable defense/upper limit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

prb112
prb112 reviewed Nov 23, 2020
View reviewed changes
Copy link
Contributor

@prb112 prb112 left a comment

Choose a reason for hiding this comment

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

LGTM - left some comments

reviewed - just a couple comments... one occurred to me at the end
1 - protection of the maximum number of reindexable resources, versus someone looping against the server with say the number 1 million
2 - Some of the SQL generation seems fit for a final constant as it's not embedding a schema.
3 - tip on string(diag) versus String.of
otherwise seems good

@punktilious
issue #1742 clamp resource count to 1000 to avoid client read timeouts
07f794b 
Signed-off-by: Robin Arnold <robin.arnold23@ibm.com>
@punktilious
issue #1742 address review comments
92bd1a7 
Signed-off-by: Robin Arnold <robin.arnold23@ibm.com>
prb112
prb112 approved these changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@prb112 prb112 left a comment

Choose a reason for hiding this comment

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

LGTM

@punktilious punktilious merged commit a02a4d2 into master Nov 23, 2020
@punktilious punktilious deleted the robin-proto branch November 23, 2020 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelwschroeder michaelwschroeder michaelwschroeder left review comments

@prb112 prb112 prb112 approved these changes

@lmsurpre lmsurpre Awaiting requested review from lmsurpre

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@punktilious @prb112 @michaelwschroeder