Use new document store search api #255
Conversation
| @@ -202,12 +209,11 @@ public Map<ConfigResourceContext, ContextSpecificConfig> getContextConfigs( | |||
| @Override | |||
| public List<ContextSpecificConfig> getAllConfigs(ConfigResource configResource) | |||
| throws IOException { | |||
| Query query = new Query(); | |||
| query.setFilter(this.getConfigResourceFilter(configResource)); | |||
There was a problem hiding this comment.
If getConfigResourceFilter isn't getting used anywhere else, can we get rid of it?
| Query query = new Query(); | ||
| query.setFilter(this.getConfigResourceFilter(configResource)); | ||
| query.addOrderBy(new OrderBy(VERSION_FIELD_NAME, false)); | ||
| org.hypertrace.core.documentstore.query.Query query = buildQuery(configResource); |
There was a problem hiding this comment.
If we have replaced all instance of Query, then, we can directly import this Query class instead of fully-qualifying it every time?
There was a problem hiding this comment.
Old api was getting some other places also, moved to new one everywhere
| @@ -94,7 +102,7 @@ public UpsertedConfig writeConfig( | |||
| .asRuntimeException(); | |||
| } | |||
| } else { | |||
| collection.upsert(latestDocKey, latestConfigDocument); | |||
| collection.createOrReplace(latestDocKey, latestConfigDocument); | |||
There was a problem hiding this comment.
@suresh-prakash
This is the change i had made few months back and then reverted cause of issue in downstream doc store api .
It was fixed in hypertrace/document-store#207
but recently we revert this too (cause of issue in Iam service we saw recently) hypertrace/document-store#213
we would want to be sure on this
There was a problem hiding this comment.
@harshit-kumar-v2 There is no issue with the createOrReplace API itself. The problem is only when we have a value that starts with a dollar. That case is not handled and we haven't faced a real use case for it. So, this change is safe.
There was a problem hiding this comment.
It's not safe because the values are provided by external users - same thing we discussed last time this went in and was reverted. So we need to revert this again until we can prioritize fixing doc store @suresh-prakash.
There was a problem hiding this comment.
Ah didn't mean to pile on, just came across the already-existing comments on the revert PR! Any further discussion can be had there.
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.