Conversation
cwlacewe
left a comment
There was a problem hiding this comment.
Looks good to me. Minor removal of extra lines requested.
This is a concern of mine longer term, but it doesn't look like the "delete" is actually externally connected at the moment. i.e. I dont see any VDMS level logic that lets us fully remove an index. At most, with some gymnastics we might be able to remove the descriptor set reference from the graph database, but even thats a little murky. @s-gobriel do you know if there's an example or call we can check to see if a client can actually trigger a delete of either the index or the referencing PMGD node? |
formatting cleanup Co-authored-by: Chaunte W. Lacewell <chaunte.w.lacewell@intel.com>
s-gobriel
left a comment
There was a problem hiding this comment.
You are correct @ifadams. I checked the whole source and the memory resident index is never deleted, so I don't see a problem with the cached path.
|
Target CPP Coverage: 64.4418% Target Python Coverage: 98% |
|
Target CPP Coverage: 64.4418% Target Python Coverage: 98% |
src/RSCommand.h
Outdated
| const std::string _cmd_name; | ||
| std::map<std::string, int> _valid_params_map; | ||
|
|
||
| static tbb::concurrent_unordered_map<std::string, std::string> |
There was a problem hiding this comment.
Is there a reason to place this DescriptorSet specific cache in the base RSCommand class? Won't it be better placed in DescriptorsCommand class?
There was a problem hiding this comment.
At the time there was a specific reason (actually did the dev work weeks ago) and was having some compilation issues with it in the subclass.
I can't for the life of me remember the specifics though.... if its causing some heartburn I can try moving it back to the subclass. There's some vaguely potential value of having it in the super class for more general access for some hand-wavey other future dev but from a functional standpoint right now, meh
|
Target CPP Coverage: 64.4418% Target Python Coverage: 98% |
|
Target CPP Coverage: 64.4418% Target Python Coverage: 98% |
|
Target CPP Coverage: 64.4418% Target Python Coverage: 98% |
|
Target CPP Coverage: 64.4418% Target Python Coverage: 98% |
Hey Folks,
This is a draft PR for the descriptor set path caching.
Currently, to the best of my knowledge, we can't actually delete a descriptor set after creation so this is a fairly minimal change.
It relies on the fact that a descriptor set path, as well as its dimensionality, will never change after creation. This lets us bypass the database query after the initial caching of the path and dimensions for a named set.