Skip to content

Allow users to customize Client Connection #2734

Closed
nmengin wants to merge 1 commit intomoby:masterfrom
nmengin:2733-customize-grcp-client-dial-options
Closed

Allow users to customize Client Connection #2734
nmengin wants to merge 1 commit intomoby:masterfrom
nmengin:2733-customize-grcp-client-dial-options

Conversation

@nmengin
Copy link
Copy Markdown
Contributor

@nmengin nmengin commented Aug 29, 2018

Signed-off-by: nmengin nicolas@containo.us

- What I did

Adding an option to allow users to give DialOptions to the ConnectionBroker and raft.Transport gRPC client connection.

- How I did it

By creating an option into the node.Node Configuration.

- How to test it

I modified the test to add DialOption.

- Description for the changelog

Adding an entry into the Node configuration to allow users to customize the DialOptions for the ConnectionBroker and raft.Transport gRPC client connection.

Fixes #2733

@nmengin nmengin force-pushed the 2733-customize-grcp-client-dial-options branch 2 times, most recently from c42473d to 035160c Compare August 29, 2018 16:30
@nmengin
Copy link
Copy Markdown
Contributor Author

nmengin commented Aug 29, 2018

I guess the CI is down because of the problem described in #2728 .

@vdemeester
Copy link
Copy Markdown
Member

@olljanat
Copy link
Copy Markdown
Contributor

@nmengin is this one still valid? Can you rebase?

@nmengin nmengin force-pushed the 2733-customize-grcp-client-dial-options branch from 035160c to 60d2c7c Compare November 28, 2018 19:52
@nmengin
Copy link
Copy Markdown
Contributor Author

nmengin commented Nov 28, 2018

Hello @olljanat , yes the PR is still valid.
PR rebased and tested locally.

I hope the CI will validate it 😄

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 28, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@08d6e4a). Click here to learn what that means.
The diff coverage is 95.83%.

@@            Coverage Diff            @@
##             master    #2734   +/-   ##
=========================================
  Coverage          ?   62.02%           
=========================================
  Files             ?      137           
  Lines             ?    22136           
  Branches          ?        0           
=========================================
  Hits              ?    13729           
  Misses            ?     6930           
  Partials          ?     1477

@olljanat
Copy link
Copy Markdown
Contributor

@dperny @wk8 PTAL

…anism

Signed-off-by: nmengin <nicolas@containo.us>
@nmengin nmengin force-pushed the 2733-customize-grcp-client-dial-options branch from 60d2c7c to 2467537 Compare January 3, 2019 14:48
@nmengin
Copy link
Copy Markdown
Contributor Author

nmengin commented Jan 4, 2019

Hello @olljanat , @dperny , @wk8

The CI failed since I rebased my branch. I haven't reproduced the problem locally.
Do you have any idea of the problem?

I can help to analyze if necessary.

@olljanat
Copy link
Copy Markdown
Contributor

olljanat commented Jan 4, 2019

I think that it is flaky test #2559

@nmengin
Copy link
Copy Markdown
Contributor Author

nmengin commented Jan 4, 2019

Ok @olljanat , thank you for the response.

Do you think it's possible to relaunch the CI or do you prefer I make a push?

@thaJeztah
Copy link
Copy Markdown
Member

I kicked the build

@nmengin
Copy link
Copy Markdown
Contributor Author

nmengin commented Jan 4, 2019

Thank you @thaJeztah .

For information, do yo think this PR might be merged soon or there is a design problem with it?

@olljanat
Copy link
Copy Markdown
Contributor

olljanat commented Jan 4, 2019

@nmengin did I understood right that you actually want give user possibility to workaround #2733 ? If that is case I guess that this will not get approval from maintainers.

Related issues:

and afaik most technical discussion are done on #2774

@nmengin
Copy link
Copy Markdown
Contributor Author

nmengin commented Jul 12, 2019

Hello @olljanat .

Unfortunately the workaround you proposed did not work.
Indeed, when I create a huge number of Swarmkit objects, the nodes send the delta between the old configuration and the new one to the managers.
But the broker refuses to receive these messages because the maximum size for the messages it can received is the default one (4194304 byte) and the nodes send bigger messages.

I guess this PR will not be merged, so I close it.
But I just opened another (smaller) one to fix the initial issue #2869, don't hesitate to take a look if you want.

@nmengin nmengin closed this Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error generated when messages size is too big

4 participants