issue 2283 - eliminate redundant reads during ingestion #2426
Conversation
Signed-off-by: Robin Arnold <robin.arnold23@ibm.com>
...rsistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java
Show resolved
Hide resolved
punktilious
requested review from
lmsurpre,
JohnTimm and
tbieste
and removed request for
michaelwschroeder
| Resource contextResource) throws Exception { | ||
| return doRead(type, id, throwExcOnNull, includeDeleted, contextResource, null); | ||
| } | ||
|
|
||
| @Override | ||
| public Resource doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted, | ||
| public SingleResourceResult<? extends Resource> doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted, |
There was a problem hiding this comment.
Do we still need to pass includeDeleted now that we have the isDeleted marker in the response?
There was a problem hiding this comment.
It would be possible to refactor the entire handling of deleted status, but this is a much bigger task. Currently the persistence layer will throw an exception to signal the resource has been deleted. Handling this without using an exception is non-trivial.
There was a problem hiding this comment.
Currently the persistence layer will throw an exception to signal the resource has been deleted.
but only conditionally, based on this flag.
I guess was was hoping we could maybe get away with only breaking this method signature once and remove includeDeleted now. I'm fine with saving that for "future work" and I agree its probably not a priority.
fhir-server/src/main/java/com/ibm/fhir/server/operation/spi/FHIRResourceHelpers.java
Outdated
Show resolved
Hide resolved
fhir-server/src/main/java/com/ibm/fhir/server/operation/spi/FHIRResourceHelpers.java
Outdated
Show resolved
Hide resolved
| @@ -392,6 +393,9 @@ private FHIRRestOperationResponse doPatchOrUpdate(String type, String id, FHIRPa | |||
| } else { | |||
| id = newResource.getId(); | |||
| } | |||
|
|
|||
| // No match, so deletion status doesn't matter | |||
| isDeleted = false; | |||
There was a problem hiding this comment.
The boolean is already going to be false by default, but I guess it doesn't hurt to do this for clarity (same on line 415) along with the comment.
There was a problem hiding this comment.
yeah, i actually prefer explicitly initializing it (even though it is redundant)...just one less thing to remember :-)
There was a problem hiding this comment.
Actually it's not initialized on purpose:
boolean isDeleted; // stash the deleted status of the resource when we first read it
I do this intentionally to make sure I set it in each of the if/then/else paths. If I miss a path, the compiler will catch it.
There was a problem hiding this comment.
I think that is true for Boolean fields, but is it true for booleans?
There was a problem hiding this comment.
Default is false if it's a member of the class. But for local variables in the method, default is unassigned (Eclipse will complain too if you try to use it without assigning a value).
Signed-off-by: Robin Arnold <robin.arnold23@ibm.com>
issue 2283 - eliminate redundant reads during ingestion
Signed-off-by: Robin Arnold robin.arnold23@ibm.com