Skip to content
This repository has been archived by the owner on Dec 5, 2021. It is now read-only.

address an issue with the datacenter selection... #938

Merged
merged 1 commit into from
Apr 11, 2020
Merged

address an issue with the datacenter selection... #938

merged 1 commit into from
Apr 11, 2020

Conversation

solarin
Copy link
Contributor

@solarin solarin commented Apr 11, 2020

that could select datacenters which were not supporting standard communication

@solarin
Copy link
Contributor Author

solarin commented Apr 11, 2020

@knocte is my concern clear now why we should push the other fix first?

@knocte
Copy link
Collaborator

knocte commented Apr 11, 2020

not clear man, what would happen if I merge only the line that filters the MediaOnly DCs and not the session save? what happens in that case?

@solarin
Copy link
Contributor Author

solarin commented Apr 11, 2020

it works. then when you push fix 937 it won't work. because you need the line session.save

@solarin
Copy link
Contributor Author

solarin commented Apr 11, 2020

i don't know what else i can say, try it if you don't believe/understand/unsure of what i am saying. i have been debugging for hours and now also documenting it for many more hours.
i might sound arrogant, but i value my time a lot.

@knocte
Copy link
Collaborator

knocte commented Apr 11, 2020

but i value my time a lot.

It's not that I don't believe you, it's that I want to understand before merging.

it works. then when you push fix 937 it won't work. because you need the line session.save

Well, then I will merge first a commit filtering the MediaOnly, whose commits message says that it fixes bug #935. After that, I'll modify PR 937 to include that Save() call. And we're done. And each commit fixes a separate thing. Ok with this?

@@ -134,8 +136,9 @@ private async Task ReconnectToDcAsync(int dcId, CancellationToken token = defaul

var dataCenter = new DataCenter (dcId, dc.IpAddress, dc.Port);

transport = new TcpTransport(dc.IpAddress, dc.Port, handler);
transport = new TcpTransport(dc.IpAddress, dc.Port, handler); // this will be gone after fix #937
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you keep this line after fix 937, it will be useless because the connectionAsync on line 140 will cause a recreation of the TcpTransport

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right so then this has to be fixed in the PR#937, not in the same commit that does the MediaOnly filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok then
let's go ahead.

nothing personal, ok, i am a very direct and at the same time sincere person.

cheers

session.DataCenter = dataCenter;
session.Save(); // this is needed after fix #937
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you don't save the session, the new tcpTransport created inside connectAsync (firx 937), will reload the session with the old datacenter value

@knocte knocte merged commit 9a6e391 into sochix:master Apr 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants