Skip to content

Conversation

@WRadoslaw
Copy link
Contributor

No description provided.

# Conflicts:
#	.env
#	Makefile
#	db/migrations/1704717446430-Data.js
#	package-lock.json
#	package.json
#	src/mappings/content/channel.ts
#	src/mappings/content/video.ts
#	src/processor.ts
#	src/server-extension/resolvers/VideosResolver/index.ts
#	src/server-extension/resolvers/VideosResolver/types.ts
#	src/utils/language.ts
@WRadoslaw WRadoslaw requested a review from zeeshanakram3 April 8, 2024 19:19
Comment on lines +49 to +59
// setup item properties
const itemPropertiesReqs = Object.entries(itemPropToType).map(
([propName, type]) => new requests.AddItemProperty(propName, type)
)

// setup segments
const segmentsReqs = new requests.CreatePropertyBasedSegmentation(
'channel-segmentation',
'items',
'channel_id'
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoudn't there be a check before creating properties to ensure that the properties does not already exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recombee will just return 409 if a property is already created

Comment on lines +36 to 43

if (user && !userId) {
recommendationServiceManager.scheduleUserUpsert({ id: user.id })
}

if (!user) {
throw new UnauthorizedError('User not found by provided userId')
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (user && !userId) {
recommendationServiceManager.scheduleUserUpsert({ id: user.id })
}
if (!user) {
throw new UnauthorizedError('User not found by provided userId')
}
if (!user) {
throw new UnauthorizedError('User not found by provided userId')
}
recommendationServiceManager.scheduleUserUpsert({ id: user.id })

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the goal here was to not upsert the same anonymous user each time they enter the Atlas.

  1. No user ID in req - new user
  2. User ID in req - old anonymous user that is just looking for a new cookie

Comment on lines +47 to +52
// recommendationServiceManager.scheduleClickEvent(
// `${itemId}-video`,
// session.userId,
// duration,
// recommId
// )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this code block is commented out?

})
await row.save()

// recommendationServiceManager.scheduleItemConsumed(`${itemId}-video`, session.userId, recommId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is commented out?

Comment on lines +40 to +45
// recommendationServiceManager.scheduleViewPortion(
// `${itemId}-video`,
// session.userId,
// portion,
// recommId
// )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this code block is commented out?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is there a need for this handler since video rating is being set from processVideoReaction mapping>

Comment on lines 385 to 395
private async _batchUpdateLoop(intervalMs: number): Promise<void> {
while (true) {
if (this._queue.length) {
const batchArray = [...this._queue]
this._queue.length = 0
await this.sendBatchRequest(batchArray)
}
await new Promise((resolve) => setTimeout(resolve, intervalMs))
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, a general implementation problem with this approach is that you are not persisting the items/interactions pushed to the queue(it's all in-memory). So, if sendBatchRequest fails or the process (Graphql Server, processor or Auth-API) fails with some elements in the queue, the elements will be lost forever.

Fix

I would suggest you go with some persistent queue options.

@WRadoslaw
Copy link
Contributor Author

  1. Commented code and new up-squid command - I remember that we agreed to use mongo db to save user interactions only for the initial period before we start using Recombee all the way, that's why I just commented out some code in the interactions API handler (if I were to implement it in RecommendationServiceManager both processor and auth API would have to init connection to mongo db, which isn't necessary). Is it the best approach? It's not, but under the assumption that after a month or two we would just delete the mongo related code I'd say it's enough.

After we make a switch we can make sure to start only interactions API when both variables are present. Now we need to launch these services even if Recombee cannot be setup, BCS of mongo db interactions.

  1. Persisted queue - discussion in progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants