Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

Client support multiple servers #658

Merged
merged 12 commits into from
Sep 21, 2020
Merged

Client support multiple servers #658

merged 12 commits into from
Sep 21, 2020

Conversation

wision
Copy link
Contributor

@wision wision commented Sep 16, 2020

Resolves: #408

UziTech
UziTech suggested changes Sep 16, 2020
View reviewed changes
docs/client.md Outdated Show resolved Hide resolved
UziTech
UziTech reviewed Sep 16, 2020
View reviewed changes
lib/client/client.js Outdated Show resolved Hide resolved
jsumners
jsumners reviewed Sep 16, 2020
View reviewed changes
lib/client/client.js Outdated Show resolved Hide resolved
@UziTech
Copy link
Member

UziTech commented Sep 16, 2020

@wision what is the purpose of the multiple servers in this PR. Is it for redundancy, lower latency, other?

@jsumners
Copy link
Member

I think this PR is looking pretty good. We need to address feedback and add some tests.

@wision
Copy link
Contributor Author

wision commented Sep 17, 2020 •
edited
Loading

@wision what is the purpose of the multiple servers in this PR. Is it for redundancy, lower latency, other?

The reason is redundancy / failover. We have three ldap servers and it became important that we keep serving requests even if one of the servers is down (e.g. maintenance or similar).

@wision wision requested a review from UziTech September 17, 2020 05:08
@wision
Copy link
Contributor Author

wision commented Sep 17, 2020

Anyone has an idea how to fix that windows test? Before adding connectTimeout it happened that sometimes the test passed and sometimes it failed (I'm using debian bullseye & nodejs 14). After adding connectTimeout I ran the test ~20 times and it always passed.. but it seems that windows still has some problems.

@UziTech
Copy link
Member

UziTech commented Sep 17, 2020

The reason is redundancy / failover.

Ok. When I first saw "multiple servers" I thought it would be for lower latency. I thought it would try to connect to all of the servers at once and choose the first one that responds.

UziTech
UziTech suggested changes Sep 17, 2020
View reviewed changes
lib/client/client.js Outdated Show resolved Hide resolved
lib/client/client.js Show resolved Hide resolved
lib/client/client.js Outdated Show resolved Hide resolved
UziTech
UziTech reviewed Sep 17, 2020
View reviewed changes
lib/client/client.js Outdated Show resolved Hide resolved
UziTech
UziTech suggested changes Sep 17, 2020
View reviewed changes
lib/client/client.js Outdated Show resolved Hide resolved
UziTech
UziTech suggested changes Sep 17, 2020
View reviewed changes
lib/client/client.js Outdated Show resolved Hide resolved
UziTech
UziTech suggested changes Sep 17, 2020
View reviewed changes
lib/client/client.js Show resolved Hide resolved
UziTech
UziTech reviewed Sep 17, 2020
View reviewed changes
lib/client/client.js Outdated Show resolved Hide resolved
UziTech
UziTech suggested changes Sep 18, 2020
View reviewed changes
test/client.test.js Outdated Show resolved Hide resolved
@wision @UziTech
Change test ip address.
85824f8 
Co-authored-by: Tony Brix <tony@brix.ninja>
UziTech
UziTech approved these changes Sep 18, 2020
View reviewed changes
Copy link
Member

@UziTech UziTech left a comment

Choose a reason for hiding this comment

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

LGTM

jsumners
jsumners reviewed Sep 18, 2020
View reviewed changes
lib/client/client.js Outdated Show resolved Hide resolved
jsumners
jsumners approved these changes Sep 18, 2020
View reviewed changes
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

LGTM

I'll merge release likely this weekend.

@UziTech
Copy link
Member

UziTech commented Sep 18, 2020

It might be worth it to create a demo in the docs of how someone could randomize the array before sending it to ldapjs in order to achieve the other strategy.

@jsumners jsumners merged commit fadf3fc into ldapjs:master Sep 21, 2020
@jsumners jsumners mentioned this pull request Dec 13, 2020
Closed
rkaw92 added a commit to rkaw92/DefinitelyTyped that referenced this pull request Jul 14, 2021
@rkaw92
[ldapjs] Add support for multiple URLs
6723d73 
This change adds support for passing an array instead of a single URL
to createClient().

Added upstream in ldapjs/node-ldapjs#658
@rkaw92 rkaw92 mentioned this pull request Jul 14, 2021
Merged
8 tasks
typescript-bot pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jul 16, 2021
@rkaw92
🤖 Merge PR #54515 [ldapjs] Add support for multiple URLs by @rkaw92
89a78c5 
* [ldapjs] Add support for multiple URLs

This change adds support for passing an array instead of a single URL
to createClient().

Added upstream in ldapjs/node-ldapjs#658

* [ldapjs] Update the long-outdated type definitions header

* [ldapjs] Lower version in type header to 2.2

This brings the typing in accordance with when the "multiple URLs" feature
was actually introduced.
@jsumners
Copy link
Member

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.

@ldapjs ldapjs locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@UziTech UziTech UziTech approved these changes

@jsumners jsumners jsumners approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support multiple servers in the client
3 participants
@wision @UziTech @jsumners