fix: add delete() for cached property

[cristianmtr]

• provide overridden method for del for @cached_property
• correct closing of query handler in BinaryPb

NOTE

@florian-hoenicke @JoanFM @maximilianwerk

Closing the query_handler is now coherent, but makes the update and delete ops slower. This is a trade-off we need to discuss perhaps.

Before, we were only closing the qhandler's body but still relying on its header to get the keys. That was a bit of smell due to the design of the Write/ReadHandlers in BinaryPb. Once we call close we shouldn't be able to access it, but we are.

Do we want to keep the old way (less readable, but more efficient), or this way (more readable, less efficient)?

[codecov]

Codecov Report

Merging #2144 (147b3f6) into master (441966b) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2144      +/-   ##
==========================================
- Coverage   90.38%   90.37%   -0.02%     
==========================================
  Files         211      211              
  Lines       11199    11205       +6     
==========================================
+ Hits        10122    10126       +4     
- Misses       1077     1079       +2     

Flag	Coverage Δ
daemon	50.38% <7.14%> (-0.02%)	⬇️
jina	90.82% <100.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
jina/executors/indexers/keyvalue.py	98.37% <100.00%> (ø)
jina/executors/indexers/vector.py	95.68% <100.00%> (ø)
jina/helper.py	83.65% <100.00%> (+0.21%)	⬆️
jina/peapods/runtimes/zmq/zed.py	91.48% <0.00%> (-1.42%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 441966b...147b3f6. Read the comment docs.

[github-actions]

Latency summary

Current PR yields:

• 😶 index QPS at 1006, delta to last 3 avg.: -3%
• 😶 query QPS at 17, delta to last 3 avg.: -3%

Breakdown

Version	Index QPS	Query QPS
current	1006	17
1.0.8	1059	17
1.0.7	1030	17

Backed by latency-tracking. Further commits will update this comment.

[cristianmtr]

@JoanFM (not sure why I can't quote your comments)

I think this is a code smell that we are caching something we should not cache

Yes, that is a smell buried deep in Jina unfortunately. We are caching the query handler, for example in here

jina/jina/executors/indexers/__init__.py

Line 85 in 640daf4

@cached_property

I see why this is necessary as we want to avoid the performance hit of having to build it up every time.

not really a fan of this to be honest. We assume that close is an special method, which may not be true

Hm I see. Is there any alternative you can think of? We could check that the field name is something like 'query handler' etc., but that would be just as bad.

We are making a similar assumption here:

jina/jina/executors/indexers/__init__.py

Line 169 in 640daf4

call_obj_fn(self.write_handler, 'close')

[JoanFM]

@JoanFM (not sure why I can't quote your comments)

I think this is a code smell that we are caching something we should not cache

Yes, that is a smell buried deep in Jina unfortunately. We are caching the query handler, for example in here

jina/jina/executors/indexers/__init__.py

Line 85 in 640daf4

@cached_property

I see why this is necessary as we want to avoid the performance hit of having to build it up every time.

not really a fan of this to be honest. We assume that close is an special method, which may not be true

Hm I see. Is there any alternative you can think of? We could check that the field name is something like 'query handler' etc., but that would be just as bad.

We are making a similar assumption here:

jina/jina/executors/indexers/__init__.py

Line 169 in 640daf4

call_obj_fn(self.write_handler, 'close')

Can't we maybe have a private class that can control if a handler is to be reread or recomputed or something?

[cristianmtr]

Can't we maybe have a private class that can control if a handler is to be reread or recomputed or something?

Not sure I understand what you mean. Another class decorator, like cached_property?

answered

[maximilianwerk]

Can't we maybe have a private class that can control if a handler is to be reread or recomputed or something?

What we could do is to have an argument for the decorator, which says, if it is closable of not in order to make it explicit instead of implicit. Anyhow, the current solution is fine in my opinion.

[maximilianwerk]

LGTM