Skip to content

Feature/no connection serial recovery key - #980

Merged
sacOO7 merged 23 commits into
feature/no-connection-serialfrom
feature/no-connection-serial-recovery-key
Dec 1, 2023
Merged

Feature/no connection serial recovery key#980
sacOO7 merged 23 commits into
feature/no-connection-serialfrom
feature/no-connection-serial-recovery-key

Conversation

@sacOO7

@sacOO7sacOO7 commented Nov 27, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Related #974
Related #846
Related #976

@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features November 27, 2023 12:07 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc November 27, 2023 12:07 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features November 28, 2023 12:53 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc November 28, 2023 12:54 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features November 28, 2023 13:00 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc November 28, 2023 13:01 Inactive
@sacOO7
sacOO7force-pushed the feature/no-connection-serial-recovery-key branch from 229617e to 1509938CompareNovember 28, 2023 13:01
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features November 28, 2023 13:01 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc November 28, 2023 13:02 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features November 28, 2023 16:48 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc November 28, 2023 16:48 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features November 28, 2023 17:04 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc November 28, 2023 17:04 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features November 28, 2023 17:29 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc November 28, 2023 17:29 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc November 29, 2023 12:26 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features November 29, 2023 12:45 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc November 29, 2023 12:45 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features November 29, 2023 13:01 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc November 29, 2023 13:01 Inactive
@sacOO7
sacOO7force-pushed the feature/no-connection-serial branch from 6461967 to 72992c9CompareNovember 29, 2023 17:14
@sacOO7
sacOO7 requested review from lmars and ttypicNovember 29, 2023 17:19
@sacOO7
sacOO7 marked this pull request as ready for review November 29, 2023 17:20
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features November 30, 2023 12:26 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc November 30, 2023 12:27 Inactive
ttypic
ttypic reviewed Nov 30, 2023
View reviewed changes
Comment threadlib/src/main/java/io/ably/lib/realtime/AblyRealtime.java Outdated
Comment threadlib/src/main/java/io/ably/lib/types/RecoveryKeyContext.java
public RecoveryKeyContext(String connectionKey, long msgSerial, Map<String, String> channelSerials) {
this.connectionKey = connectionKey;
this.msgSerial = msgSerial;
this.channelSerials.putAll(channelSerials);

@ttypicttypicNov 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why do we copy channelSerials?

@sacOO7sacOO7Dec 1, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

General principal of immutability. We have marked channelSerials field as immutable, so it shouldn't get influenced by
changes in external channelserials and should only capture snapshot of serials at the time of creating the instance.

@ttypicttypicDec 1, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Got you! (one more point to rewrite lib in Kotlin) I personally prefer using analog of guava's ImmutableMap which just wraps around existing Map rather than make Map's copy. But it this particular code probably no harm to copy

Comment threadlib/src/main/java/io/ably/lib/transport/ConnectionManager.java
Comment threadlib/src/main/java/io/ably/lib/realtime/ChannelBase.java
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features December 1, 2023 11:10 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc December 1, 2023 11:11 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/features December 1, 2023 11:15 Inactive
@github-actions
github-actionsBottemporarily deployed to staging/pull/980/javadoc December 1, 2023 11:15 Inactive
@sacOO7
sacOO7 merged commit 47862f4 into feature/no-connection-serialDec 1, 2023
@sacOO7
sacOO7 deleted the feature/no-connection-serial-recovery-key branch December 1, 2023 15:48
@sacOO7sacOO7 mentioned this pull request Mar 11, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ttypicttypicttypic approved these changes
@owenpearsonowenpearsonAwaiting requested review from owenpearson
@lmarslmarsAwaiting requested review from lmars

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sacOO7@ttypic