Add new API to list paginated qualified conv ids by akshaymankar · Pull Request #1686 · wireapp/wire-server

akshaymankar · 2021-07-28T15:06:02Z

More details: https://wearezeta.atlassian.net/wiki/spaces/CORE/pages/477660475/Federated+Conversations+Pagination

Checklist

It doesn't fail the way I want it tho!

pcapriotti

This looks good, but I would suggest going the extra mile and try improving the way pagination works, since we are changing the API anyway.
Specifically, I think it would be nice if we could, at the same time:

get rid of the max + 1 trick, since getting an extra empty page at the end is not a big problem, and only happens if the page size happens to divide the total number of conversations;
actually use Cassandra's pagination support, instead of doing it by hand; then we wouldn't need to make two queries to paginate remote conversations;
get rid of the awkward ResultSet type.

To that end, instead of using paginate, we can directly extract the Metadata field from the result of a query, and look at the pagingState value in there (see the code of paginate itself). This value can be returned to the client, and later passed to subsequent queries. Of course, we would need to augment it with a bit that tells us whether we need to resume a local conversation query or a remote one.

I haven't looked at the tests too carefully, but it looks like they are pretty thorough. Minor comments follow.

services/galley/src/Galley/API/Query.hs

pcapriotti · 2021-08-04T05:47:22Z

services/galley/src/Galley/Data.hs

+-- Otherwise, reads 'max' records starting from the 'start' parameter. Doing
+-- this is unfortunately not trivial, so this function first gets all the
+-- conversations which match the domain and then if there is still space, gets
+-- the conversations which have domain > domain of start.


I imagine this is necessary because there is no way in cassandra to say (domain, id) > (start_domain, start_id)?

Not exactly this, but there is a token function. But its behaviour depends on some Cassandra settings, I am going to try to use it and if it works, maybe explore how Cassandra will behave under different settings.

From my reading so far token can only be used on "partitiion keys". Partition keys are keys which are listed in the first column of the primary key. Right now our primary key is (user, remote_conv_domain, remote_conv_id), so our primary key is just user. If we want this to work, I guess we'll have to change this to something more complex like ((user, remote_conv_domain, remote_conv_id), user). And then set the cluster ordering by user. This may allow us to write queries like WHERE token(user, remote_conv_domain, remote_conv_id) > (?, ?, ?) AND user = ?. Not sure what other problems this might cause, I am already thinking this might cause data for one user to be scattered across different nodes, which might not be ideal. I am gonna read more.

Actually, the partition key doesn't need to be unique, so we can even get rid of user from it, making our primary key to be ((remote_conv_domain, remote_conv_id), user)

But why not use pagingState instead and let the driver handle it? Or am I missing something?

But why not use pagingState instead and let the driver handle it? Or am I missing something?

I will try this next and see if it works.

This actually works 🎉

pcapriotti · 2021-08-04T05:48:21Z

services/galley/src/Galley/Data.hs

+          nextPage <- toResultSet remainingMax <$> paginate Cql.selectUserRemoteConvsFromDomain (paramsP Quorum (usr, d) (remainingMax + 1))
+          pure $ nextPage {resultSetResult = resultSetResult domainPage <> resultSetResult nextPage}


This idiom of using max + 1 then discarding the last element is used in multiple places, so it would be good to extract it in a convenience function.

This is no longer required, so maybe we can just retire this thing soon.

services/galley/test/integration/API.hs

mdimjasevic

I can't say I understand the code well. I guess the main sections are in the Data and Query modules, but I don't understand them properly. If no one else approves the PR, let me know and let's go through the code together.

Smaller observations are inlined in the code.

services/galley/test/integration/API/Util.hs

services/galley/test/integration/API.hs

Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>

mdimjasevic

This looks good! I left minor comments and some questions inlined.

mdimjasevic · 2021-08-10T05:49:17Z

libs/wire-api/src/Wire/API/Conversation.hs

+        <*> pageHasMore .= field "has_more" schema
+        <*> pagePagingState .= field "paging_state" schema
+
+-- | TODO: Would be nice to not expose these details to clients


Are you planning to take care of the TODO as part of this PR? If not, it should be a future work note.

Ah yes, I forgot to ask y'all if you had any better ideas. Do you?

I implemented Paolo's idea of encoding it as a byte and putting it in front of the state, let me know if that looks ok.

libs/wire-api/src/Wire/API/Conversation.hs

mdimjasevic · 2021-08-10T05:55:17Z

libs/wire-api/src/Wire/API/Conversation.hs

+
+instance ToSchema ConversationPagingTable where
+  schema =
+    (S.schema . description ?~ "Used getting PagedConv") $


I do not understand/parse this description. Can you expand it?

A left over rename and missing 'for', I will rewrite this, thanks 👍

mdimjasevic · 2021-08-10T06:15:18Z

services/galley/src/Galley/API/Query.hs

+      let remainingSize = fromRange size - fromIntegral (length (Public.pageConvIds localPage))
+      if Public.pageHasMore localPage


I haven't looked at tests yet, but have you covered different cases that depend on the remaining size and if there is more to list?

I am not sure if I understand, can you please elaborate?

mdimjasevic · 2021-08-10T06:17:23Z

services/galley/src/Galley/API/Query.hs

+
+    localsAndRemotes :: Domain -> Maybe C.PagingState -> Range 1 1000 Int32 -> Galley Public.ConvIdsPage
+    localsAndRemotes localDomain pagingState size = do
+      localPage <- pageToConvIdPage Public.PagingLocals . fmap (`Qualified` localDomain) <$> Data.conversationIdsPageFrom zusr pagingState size


The remote counterpart to Data.conversationIdsPageFrom is Data.remoteConversationIdsFrom, right? If so, how about calling it Data.localConversationIdsFrom or similar?

I renamed them to localConversationIdsPageFrom and remoteConversationIdsPageFrom, so we keep page and local or remote both.

services/galley/src/Galley/API/Query.hs

services/galley/src/Galley/Data.hs

services/galley/src/Galley/Data/Queries.hs

mdimjasevic · 2021-08-10T06:35:12Z

services/galley/test/integration/API.hs

+  -- should get all them in 16 times.
+  foldM_ (getChunkedConvs 16 0 alice) Nothing [16, 15 .. 0 :: Int]
+
+-- This test exists


Are you saying it is a duplicate and of which one?

Maybe I started writing the comment and something distracted me 😅

mdimjasevic · 2021-08-10T06:37:22Z

services/galley/test/integration/API.hs

+
+  foldM_ (getChunkedConvs 16 0 alice) Nothing [4, 3, 2, 1, 0 :: Int]
+
+-- | Gets chucked conversation ids given size of each chunk, size of the last


Suggested change

-- | Gets chucked conversation ids given size of each chunk, size of the last

-- | Gets chunked conversation ids given size of each chunk, size of the last

pcapriotti

Looks great!
I would like the paging state token to be returned a single obviously opaque bytestring, instead of an object someone might be tempted to peer into, but it's not a big deal.

I've left a few minor comments and fixes.

pcapriotti · 2021-08-10T06:26:11Z

libs/cassandra-util/src/Cassandra/Exec.hs

+-- term storage as the bytestring format may change useless when schema of a
+-- table changes or when cassandra is upgraded.


Suggested change

-- term storage as the bytestring format may change useless when schema of a

-- table changes or when cassandra is upgraded.

-- term storage as the bytestring format may change when the schema of a

-- table changes or when cassandra is upgraded.

pcapriotti · 2021-08-10T06:40:21Z

libs/wire-api/src/Wire/API/Conversation.hs

+        <*> pageHasMore .= field "has_more" schema
+        <*> pagePagingState .= field "paging_state" schema
+
+-- | TODO: Would be nice to not expose these details to clients


This could be done by encoding ConversationPagingTable as an extra bit (or byte) of cpsPagingState. I'd suggest doing this now, even though I guess doing it later won't technically break backwards compatibility.

pcapriotti · 2021-08-10T06:41:45Z

libs/wire-api/src/Wire/API/Conversation.hs

+  schema =
+    let addPagingStateDoc =
+          description
+            ?~ "optional, when not first page of the conversation ids will be returned.\


Missing "specified", I think.

pcapriotti · 2021-08-10T06:43:10Z

libs/wire-api/src/Wire/API/Routes/Public/Galley.hs

+    listConversationIds ::
+      routes
+        :- Summary "Get all conversation IDs."
+          :> Description "To retrieve next page, a client must pass the paging_state returned by previous page."


Suggested change

:> Description "To retrieve next page, a client must pass the paging_state returned by previous page."

:> Description "To retrieve the next page, a client must pass the paging_state returned by the previous page."

pcapriotti · 2021-08-10T06:46:26Z

libs/wire-api/src/Wire/API/Conversation.hs

+  schema =
+    objectWithDocModifier
+      "ConversationPagingState"
+      (description ?~ "Clients should treat this object as opque and not try to parse it.")


Suggested change

(description ?~ "Clients should treat this object as opque and not try to parse it.")

(description ?~ "Clients should treat this object as opaque and not try to parse it.")

pcapriotti · 2021-08-10T06:55:05Z

services/galley/src/Galley/API/Query.hs

-getConversationIds :: UserId -> Maybe ConvId -> Maybe (Range 1 1000 Int32) -> Galley (Public.ConversationList ConvId)
-getConversationIds zusr start msize = do
+listConversationIdsUnqualified :: UserId -> Maybe ConvId -> Maybe (Range 1 1000 Int32) -> Galley (Public.ConversationList ConvId)
+listConversationIdsUnqualified zusr start msize = do


How about converting this to use conversationIdsPageFrom, so we can remove the legacy functions and even the ResultSet type?

I like the new name, I don't know if we can easily remove the ResultSet type. I will look into it, but let's do it in the next PR?

pcapriotti · 2021-08-10T06:58:34Z

services/galley/src/Galley/API/Query.hs

+    localsAndRemotes localDomain pagingState size = do
+      localPage <- pageToConvIdPage Public.PagingLocals . fmap (`Qualified` localDomain) <$> Data.conversationIdsPageFrom zusr pagingState size
+      let remainingSize = fromRange size - fromIntegral (length (Public.pageConvIds localPage))
+      if Public.pageHasMore localPage


Would it make sense to do something like:

Suggested change

if Public.pageHasMore localPage

if Public.pageHasMore localPage or remainingSize <= 0

here to avoid having to query the other table unnecessarily?

services/galley/test/integration/API/Util.hs

akshaymankar · 2021-08-10T07:51:54Z

I would like the paging state token to be returned a single obviously opaque bytestring, instead of an object someone might be tempted to peer into, but it's not a big deal.

Do you have any ideas about this? I was thinking of just putting a slash or a dot between them. The table we control so that shouldn't be a problem and the paging state is anyway rolled into a base64 encoded string so slash or dot shouldn't collide.

Co-authored-by: Paolo Capriotti <paolo@capriotti.io>

pcapriotti · 2021-08-10T07:54:38Z

Do you have any ideas about this? I was thinking of just putting a slash or a dot between them. The table we control so that shouldn't be a problem and the paging state is anyway rolled into a base64 encoded string so slash or dot shouldn't collide.

I would add a 0 or 1 byte at the beginning (or end) of the paging state bytestring, then convert the whole thing to base 64. This could be done by the Schema instance directly.

akshaymankar · 2021-08-10T09:15:35Z

Do you have any ideas about this? I was thinking of just putting a slash or a dot between them. The table we control so that shouldn't be a problem and the paging state is anyway rolled into a base64 encoded string so slash or dot shouldn't collide.

I would add a 0 or 1 byte at the beginning (or end) of the paging state bytestring, then convert the whole thing to base 64. This could be done by the Schema instance directly.

Can you please take a look? I don't know if you meant I could parse it with the schema library, I just used attoparsec.

pcapriotti · 2021-08-10T09:20:12Z

Can you please take a look? I don't know if you meant I could parse it with the schema library, I just used attoparsec.

No, that's pretty much what I had in mind 👍

I think attoparsec is a bit overkill for this, I would have probably used binary. But it doesn't really matter.

akshaymankar marked this pull request as draft July 28, 2021 15:06

akshaymankar force-pushed the akshaymankar/paginate-remote-convs branch from cee2135 to 5cc51aa Compare August 2, 2021 13:35

akshaymankar changed the title ~~[WIP] Create new endpoint to get paginated conv ids~~ Create new endpoint to get paginated conv ids Aug 3, 2021

akshaymankar marked this pull request as ready for review August 3, 2021 13:31

akshaymankar added 9 commits August 3, 2021 15:32

[WIP] Create new endpoint to get paginated conv ids

b96e168

Fix old test so it actaully asserts

b059cfa

Add failing test

157045e

It doesn't fail the way I want it tho!

Implement paginated get remote conversations

4b096a1

Rename /convs/ids/v2 -> /convs/list-ids

febb54b

Ormolu

076cacd

Add haddocks for how the conversations are ordered

6cda742

Rename functions for consistency

082f2e8

Fix comments

5888386

akshaymankar force-pushed the akshaymankar/paginate-remote-convs branch from 227743b to 5888386 Compare August 3, 2021 13:32

akshaymankar changed the title ~~Create new endpoint to get paginated conv ids~~ Add new API to list paginated qualified conv ids Aug 3, 2021

akshaymankar added 2 commits August 3, 2021 15:35

CHANGELOG

93d84fb

Mark GET /conversations/ids as deprecated

02a8ca3

akshaymankar requested review from mdimjasevic and pcapriotti August 3, 2021 14:01

pcapriotti reviewed Aug 4, 2021

View reviewed changes

akshaymankar added 3 commits August 4, 2021 09:16

Fix typo

1967334

Remove redundant brackets

3d91255

Reduce duplication

db741e7

mdimjasevic reviewed Aug 4, 2021

View reviewed changes

akshaymankar and others added 6 commits August 4, 2021 13:33

Fix typo

064a3fc

Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>

Optimize connections

351919f

Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>

Optimize connections

18ff99c

Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>

Remove forgotten print

5b769e0

Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>

Use pagingState to page through qualified conv ids

a0dd768

Move code for paginateWithState to cassandra-util

1285218

akshaymankar added 3 commits August 9, 2021 16:11

Ormolu

3a70ff2

Add some docs and rename some vars

b5495a5

Add haddocks for 'getChuckedConvs'

da349b6

akshaymankar requested review from mdimjasevic and pcapriotti August 9, 2021 15:10

mdimjasevic approved these changes Aug 10, 2021

View reviewed changes

pcapriotti approved these changes Aug 10, 2021

View reviewed changes

Galley/int: Use new type to decode qualified conv id list

aef1401

Co-authored-by: Paolo Capriotti <paolo@capriotti.io>

akshaymankar added 4 commits August 10, 2021 10:06

Fix typos, grammar mistakes, etc

c138c4a

Make ConversationPagingState Opaque

4cb8cca

Add golden tests

007baff

Merge branch 'develop' into akshaymankar/paginate-remote-convs

c003902

akshaymankar added 4 commits August 10, 2021 11:20

Ormolu

0810f89

Better function names

8ca1d9b

Optimize number of DB calls

b66a276

Optimize type class constraints

a5f79bd

ffflorian mentioned this pull request Aug 10, 2021

fix(api-client): Update getRemoteConversations endpoint wireapp/wire-web-packages#4114

Merged

2 tasks

akshaymankar merged commit 8855f6d into develop Aug 10, 2021

akshaymankar deleted the akshaymankar/paginate-remote-convs branch August 10, 2021 14:33

fisx mentioned this pull request Aug 13, 2021

Release 2021-08-16 #1706

Merged

		nextPage <- toResultSet remainingMax <$> paginate Cql.selectUserRemoteConvsFromDomain (paramsP Quorum (usr, d) (remainingMax + 1))
		pure $ nextPage {resultSetResult = resultSetResult domainPage <> resultSetResult nextPage}

		let remainingSize = fromRange size - fromIntegral (length (Public.pageConvIds localPage))
		if Public.pageHasMore localPage


		foldM_ (getChunkedConvs 16 0 alice) Nothing [4, 3, 2, 1, 0 :: Int]

		-- \| Gets chucked conversation ids given size of each chunk, size of the last

	-- \| Gets chucked conversation ids given size of each chunk, size of the last
	-- \| Gets chunked conversation ids given size of each chunk, size of the last

		-- term storage as the bytestring format may change useless when schema of a
		-- table changes or when cassandra is upgraded.

	:> Description "To retrieve next page, a client must pass the paging_state returned by previous page."
	:> Description "To retrieve the next page, a client must pass the paging_state returned by the previous page."

	(description ?~ "Clients should treat this object as opque and not try to parse it.")
	(description ?~ "Clients should treat this object as opaque and not try to parse it.")

	if Public.pageHasMore localPage
	if Public.pageHasMore localPage or remainingSize <= 0

Conversation

akshaymankar commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

pcapriotti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdimjasevic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdimjasevic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcapriotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

akshaymankar commented Jul 28, 2021 •

edited

Loading