-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
While bringing the patch across from branch issue_13, refactored it a bit and now have a re-usable bit of code with elastic search stuff in the ElasticSearchUtil class and some PDS product stuff in buisness objects. There is a mix of stuff that uses the elastic seaarch stuff and pushes it to buisness objects that was left in the controllers. May refactor some that as it is fairly repitive too.
Moved stuff common to bundles, collections, and products to the base class.
I reworked bundles and now moving onto collections and products. Please take a look and comment please. Bundles is tested and it returns only requested fields and never loads the blob. It is highly efficient in the sense it only pulls what is needed even during the dereferencing of all the lidvid pointers. |
Oh, I will need a tutorial on why the integration is failing and how to fix it. Looks like it still wants master over main. I just do not know how to fix it. |
@al-niessner you can ignore those for now. once you get back to main and use the latest code that should be fine |
Did gov.nasa.pds.model.PropertyArrayValues; not get committed? Have it in imports but cannot be resolved. |
The code changes are basically for the walking forward/backward in the product tree more efficiently so used verify/issue_56.py to show that they still function correctly.
Ready for review with 3 caveats:
|
Both collections and products from a bundle lidvid should not work through all of the available matching data.
Removed ElasticSearchUitl.collate() and updated much of the looping so that it would find all items. It was the bulk of the work. When looking child to parent, the start and limit are used as part of the search request for which collate worked well and thus put back.
src/main/java/gov/nasa/pds/api/engineering/elasticsearch/ElasticSearchHitIterator.java
Show resolved
Hide resolved
src/main/java/gov/nasa/pds/api/engineering/controllers/MyBundlesApiController.java
Show resolved
Hide resolved
src/main/java/gov/nasa/pds/api/engineering/elasticsearch/ElasticSearchHitIterator.java
Outdated
Show resolved
Hide resolved
...va/gov/nasa/pds/api/engineering/elasticsearch/ElasticSearchRegistrySearchRequestBuilder.java
Outdated
Show resolved
Hide resolved
Still working on this problem. Got somewhat vexed today as I had the third rewrite in place and it was 10x slower. Thought I found the problem but no it was just a problem -- changing lids to lidvids is apparently expensive. Now I have some debug messages in there and need to add a couple of more to do a complete analysis (next week since I toasted 2 days this week to get here) but the current look up is now milliseconds. However, the overall time is still the same as before this optimization. Now I am going to dig deeper to find out why. Basically it takes 2 seconds to find, build, and generate 500 products. It takes about 100 ms to find the 500 products to get. I am going to look for the 1.9 seconds that I cannot account for next week. From what I have learned, it may require optimizing the lid to lidvid function. |
@al-niessner that sounds good, that is getting us closer to the goal anyway. Thanks for your feedback. |
Philosophical question: self protective coding or not? The bulk of that 1.3 seconds is turning lids to lidvids. However it may not be necessary. From the registry_ref index, product_lidvids is retrieved. Using a protective coding style out of habit, the values from product_lidvids are converted to lidvids using this.productBO.getLatestLidVidFromLid() just in case a lid made it into product_lidvids. This is 1 second of the 1.3 seconds for 500 lidvids (expensive). Now, depending on the development philosophy you want to use, there are two solutions to speed things up.
Preference? Confirmed: removing lidvid call changed from 1.3 seconds to .3 seconds which is the expected 4x speed up. |
Hi @al-niessner , I am having the following error message when I try to start the service on my latptop where an elasticsearch is on localhost with no password.
|
Sorry but I cannot reproduce the problem. Pulled the branch from github -- already up to date so no changes -- and built the docker containers. Ran the tests verify/issue_56.py which does a products of bundle with no errors. Scrolled down through the logs and found no errors like the one you have. It all works on pds-gamma as well. Maybe your configuration like the application.properties or something? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @al-niessner , I've seen 2 minor bug that we will look at (or not) after, it is not blocking the merge.
The performances are improved by 1/3 on my test (15 minutes --> 11 minutes) so that is good !
I have just a request for comments in a part of the code that I suspect will be difficult to maintain otherwise.
Thanks, sorry for the delay in the review
src/main/java/gov/nasa/pds/api/engineering/controllers/MyBundlesApiController.java
Outdated
Show resolved
Hide resolved
HashSet<String> uniqueProperties = new HashSet<String>(); | ||
List<String> productLidvids = new ArrayList<String>(); | ||
List<String> clidvids = this.getRefLidCollection(lidvid, limit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is unclear to me here as well: we are asking for product children of bundle from start to start+limit, I don't see how that can match with the collection pagination, so we can not use the limit parameter here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As state above, it is used a random number more than anything else. It could be fixed to 10 or 3 or 50 or 100. Much bigger than 100 and elasticsearch returns an error regularly.
It is an interesting question of what to set the limit of the collection of a bundle to when looking for the product.
The idea is that if the user wants to wait for bigger chunks like 100 products then also getting 100 collections is better than 10 collections then having to ask again for the other 90 in the case of one product per collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code no longer exists (outdated) so not sure what comments are required.
src/main/java/gov/nasa/pds/api/engineering/controllers/MyCollectionsApiController.java
Outdated
Show resolved
Hide resolved
src/main/java/gov/nasa/pds/api/engineering/controllers/MyCollectionsApiController.java
Outdated
Show resolved
Hide resolved
src/main/java/gov/nasa/pds/api/engineering/controllers/MyCollectionsApiController.java
Outdated
Show resolved
Hide resolved
src/main/java/gov/nasa/pds/api/engineering/controllers/MyBundlesApiController.java
Outdated
Show resolved
Hide resolved
src/main/java/gov/nasa/pds/api/engineering/controllers/MyProductsApiController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I retested the branch in pds-gamma with the notebokk and the performance are not getting better:
branch issue13.1
retrieved 279500 products in 25.9 minutes
branch main
retrieved 279500 products in 14.2 minutes
So I suggest to table this pull request for now. We can discuss that tomorrow during the breakout.
i will retest now in case there are some caching effects on elasticsearch which create a bias in the peasured performances. I'll let you know.
src/main/java/gov/nasa/pds/api/engineering/controllers/MyBundlesApiController.java
Outdated
Show resolved
Hide resolved
src/main/java/gov/nasa/pds/api/engineering/controllers/MyBundlesApiController.java
Outdated
Show resolved
Hide resolved
src/main/java/gov/nasa/pds/api/engineering/controllers/MyCollectionsApiController.java
Outdated
Show resolved
Hide resolved
When testing, just make sure network is the same. It is currently the biggest variable and causes timing to really, really swim around. I use 3 networks regularly and they all vary in how long it takes by 50% or so. If using VPN, then rate will vary minute by minute. I have not noticed an order problem (run master or 13.1 first then followed by other) in timing. Not to say that it does not exist, but I have not noticed it. |
I did re-test the new branch and had the same duration 25 minutes. So the order does not change you are right. Otherwise, per the network question, I had rather stable duration in the past and still now although I don't have a big series of tests (2 !) |
Had to undo one of your requests (see last commit) to make it fast again. |
Summary*
Brief summary of changes if not sufficiently described by commit messages.
Test Data and/or Report
One of the following should be included here:
Related Issues