fix: at_commons remove isPaginated check in sync verb builder #738
Conversation
murali-shris
changed the title
murali-shris
requested review from
gkc,
srieteja and
sitaram-kalluri
and removed request for
srieteja
There was a problem hiding this comment.
@murali-shris I know in the other comment I said a PR in at_lookup to change SyncVerbBuilder but I meant a PR in at_commons since SyncVerbBuilder is in at_commons
murali-shris
changed the title
Completed the changes. |
| if (isPaginated) { | ||
| serverCommandBuffer.write(':limit:$limit'); | ||
| } | ||
| serverCommandBuffer.write(':limit:$limit'); |
There was a problem hiding this comment.
Does the comment on line 15 mean that limit is ignored on the atServer? If so, then perhaps we shouldn't deprecate limit but instead respect it on the atServer side?
There was a problem hiding this comment.
in SyncProgressiveVerbHandler, only fromCommitId and regex are read from verb params. limit is not read.
There are two options
- rely on default limit - 25 that is configured in SyncProgressiveVerbHandler --> commitLog.getEntries and deprecate limit param in sync verb builder
- Keep limit param in sync verb builder and modify SyncProgressiveVerbHandler to read limit from verb params and pass the limit to commitLog.getEntries
There was a problem hiding this comment.
@murali-shris Option 2 please, and change the default in the SyncVerbBuilder to be in line with whatever the current default is on the server (25)
There was a problem hiding this comment.
Done.
Server PR:
atsign-foundation/at_server#2182
Client PR:
atsign-foundation/at_client_sdk#1450
- What I did
- How I did it
- How to verify it
fix: remove sync verb builder deprecated params at_client_sdk#1450