enrich disintigration #478
Conversation
| @@ -5319,17 +5319,25 @@ components: | |||
| type: string | |||
| example: "React" | |||
| description: The skill name. | |||
| UbahnSkill: | |||
| type: object | |||
| SkillInSkillsAPI: | |||
There was a problem hiding this comment.
@Sande3p do we need to specify Skills API swagger here? I mean now as api is extracted, we don't need its swagger here as well, right? or Am I missing something here?
| @@ -1270,7 +1279,7 @@ async function getSkillById (skillId) { | |||
| context: 'getSkillById', | |||
| message: `response body: ${JSON.stringify(res.body)}` | |||
| }) | |||
| return _.pick(res.body, ['id', 'name']) | |||
There was a problem hiding this comment.
Just curious why we need to remove selective picking fields? Do we need any other field where this method is being consumer?
| @@ -337,16 +337,13 @@ async function getTeam (currentUser, id) { | |||
| const teamDetail = result[0] | |||
|
|
|||
| // add job skills for result | |||
| let jobSkills = [] | |||
There was a problem hiding this comment.
@Sande3p I don't understand the need of changes in this file and as previously mentioned changes for getSkillById (removing selective picking of fields in the helper method itself rather doing it every caller) also seems to be non required to me.
| const users = res.body | ||
| // populate skill data for each user skill | ||
| await Promise.all(users.map(user => Promise.all(user.skills.map(async userSkill => { | ||
| const skill = await getSkillById(userSkill.skillId) |
There was a problem hiding this comment.
@Sande3p I think we should make a call to getAllTopcoderSkills first and then lookup for the user skills there. It would prevent multiple API calls to the skill api. If one user has 50 skills and the ubahn user query returned 100 users, it would make 5000 api calls, which is not at all feasible.
I remember seeing the cache of getAllTopcoderSkills somewhere in our code base, but not able to locate it right now. If we can have that cache loaded on app loading, it would resolve more problems. @maxceem need your thoughts here.
There was a problem hiding this comment.
@vikasrohit the previous code was already not optimized and it loaded skills individually. The only "caching" place which I know is where we are saving matching of u-bahn skills with emsi skills https://github.com/topcoder-platform/taas-apis/blob/feature/notifications-api/scripts/emsi-mapping/index.js#L50, but I doubt that could be used as a cache here.
In a nutshell, if you just would like to update TaaS API as per new Skills API, then this PR seems to be not needed, and other PR is enough https://github.com/topcoder-platform/taas-apis/pull/460/files.
But if you would like to additionally take care of performance, then suggested way with getAllTopcoderSkills might be a great idea to try. We have some pages on DEV which load a bit slow like this one https://platform.topcoder-dev.com/taas/myteams/16786, though I'm not 100% sure if it's caused by skills or no.
There was a problem hiding this comment.
@maxceem in the previous code it didn't need to be optimized because with enrich feature users objects were already having the full data they need i.e. they already have skills names in every userSkill object. So, I guess there was not a performance issue there but in the new code now we are making separate API calls to fetch the skills data.
There was a problem hiding this comment.
@vikasrohit Got it, I've missed that in this place we didn't have loading individual skills before. Then I would suggest keep the minimal changes in this PR:
- still load skills individually as it's done in all other places
- take into account you comments: most of the changes are not needed as we can
_.pick(skillObj.skill, ['id', 'name'])inside the service - if we see speed degradation in skills loading for users, then we should implement caching for all places at once by caching
getAllTopcoderSkillsand using it in all such places instead of individual calls
There was a problem hiding this comment.
@maxceem Yes, I too want the PR to be as lean as possible. However, I don't think we need to wait for testing it with respect to performance, because I don't any other place in the API where we are making similar number of API calls for fetching the individual skills. So, it has very high probability of performance impact. Isn't it? Do you see any other similar level API calls in the existing code?
There was a problem hiding this comment.
yes @vikasrohit there are such places, and they even are updated in this PR. If you check other places which are updated in this PR, there we are already requesting multiple skills by ids:
- here https://github.com/topcoder-platform/taas-apis/pull/478/files#diff-821570075559f59a1fc7d0ac9e41b013516a19e4d7a2e81c76d3b5168589c2d3L388-R388
- and there https://github.com/topcoder-platform/taas-apis/pull/478/files#diff-821570075559f59a1fc7d0ac9e41b013516a19e4d7a2e81c76d3b5168589c2d3L344-R346
Code was refactored but the call getSkillById was already there. In these places we populate Skill names for Jobs.
There was a problem hiding this comment.
Here is a picture from UI point of view:
So it may happen, that when we start loading skills individually for users too. This page might become too slow. But if we optimize such calls for users, it make sense to optimize such calls in existent places too all together.
See page example https://platform.topcoder-dev.com/taas/myteams/16786
There was a problem hiding this comment.
Great details @maxceem. That would make it clear from QA perspective as well. @lakshmiathreya @SathyaJayabal fyi.
| _.pick(skillObj.skill, ['id', 'name']) | ||
| ) | ||
| user.skills = await Promise.all((res.body.skills || []).map(async (userSkill) => { | ||
| const skill = await getSkillById(userSkill.skillId) |
There was a problem hiding this comment.
Same as with above comment for listUsersByExternalId. We should not make separate call for each skill.
vikasrohit
changed the base branch from
dev
to
feature/enriching-skills-data-with-api
|
@maxceem @nkumar-topcoder I am merging this PR to a separate feature branch as I don't have access to push changes to the forked repo from where the PR is generated. So, I will do my changes in the new feature branch and then create a new PR against dev |
vikasrohit
merged commit 47edf69
into
topcoder-platform:feature/enriching-skills-data-with-api
No description provided.