-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
Found that the API had removed the keyword in the arguments but never updated any of the code. Not sure on the correct fix, so just half removed it in this commit. If it really should be removed, then that is another ticket to finish the job.
Used the eclipse search to find all creations of the Summary then added hits and tooks to all of them.
None of the checks run because cannot reliably update the api artifact for your automatic scripts to use. Anyway, ready for review as all 4 tickets are complete and tested as shown in the initial block of this pull request. |
@al-niessner I am expecting an update of the swagger definition of the API with the new hits and took attributes. I am not seeing a branch on the swagger-ui or on the pds-qpi-javalib repository for that. The registry-api-service repo does not build without these updates apparently. Thanks, Thomas |
@al-niessner you can ignore this comment, I found the branch on swagger I did not scroll down the version menu. |
@al-niessner , @jordanpadams, I am wondering what "-1 indicating an excessive amount of resource required to compute it" does mean ? I guess we are sometimes unable to get the number of results matching the request criteria. Is that what it means ? |
There could be cases because of nested looping where it may be desirable not to spend the hours (excessive time/resources) computing the total number of hits but rather return in the millisecond of getting the information the user requested. Currently, we spend the excessive time/resources with a future option being to once again optimize it when the extra delays are better understood. I just left the -1 in the API for the future. |
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.
The keyword parameter should be added back in the code.
The naming of the variable still makes it unclear to me how the code works.
Thanks
src/main/java/gov/nasa/pds/api/engineering/controllers/MyBundlesApiController.java
Show resolved
Hide resolved
src/main/java/gov/nasa/pds/api/engineering/controllers/MyProductsApiController.java
Outdated
Show resolved
Hide resolved
This reverts commit 9090b56.
@al-niessner would you mind quickly reviewing my changes here (revert keyword removal) and here (fix merge conflicts) so we can merge the PR? |
Keyword revert is fine if changes from main support it. Problem I had was that it only appeared in code but not in api maven artifact. If it is in api maven artifact, then it should be reinstated in the code. The second change should not be returned to service. There is more comprehensive code for lid to lidvid lookups and the fix for returning 404 when lid/lidvid do not exist requires that those lines be removed. Looks like you may have a bad merge somewhere back on the main branch. |
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.
The if part is no longer desired.
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/MyBundlesApiController.java
Outdated
Show resolved
Hide resolved
The call to convert lid to lidvid is more comprehensive than the if states which in turn will break the desire that non-existent lidvid return a 404.
Fixed the fix. Should be good for a merge. |
Summary*
Added hits and took to the swagger. Adjusted the logic to fill in these items.
Test Data and/or Report
Using a standard curl, the results now show 'hits' and 'took':
Stating that there are 65 possibilities (hits) and it the processing time was 168 ms (took).
Related Issues