Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remoting: make streaming client the default #10260

Merged

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Jul 6, 2020

Problem

The streaming RE client is not the default. It has been enabled in CI for a few weeks now. New development is happening in the streaming client and not in the polling client. Time to make the streaming RE client the default.

Solution

Set the streaming RE client as the default.

Result

Nothing should break.

@tdyas tdyas changed the title remoting: default is streaming client remoting: make streaming client the default Jul 6, 2020
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Out of curiosity, do you plan to remove the polling implementation in the future?

@tdyas
Copy link
Contributor Author

tdyas commented Jul 6, 2020

Out of curiosity, do you plan to remove the polling implementation in the future?

Depends on whether we care to support Scoot at all or not. Scoot does not implement the WaitExecution method of the REv2 specification. https://github.com/twitter/scoot/blob/9b02ac8572fd634659a0dc1142692c9399d48ce1/bazel/execution/service.go#L154-L158. Thus the streaming client won't work with Scoot.

@tdyas
Copy link
Contributor Author

tdyas commented Jul 6, 2020

isort failure:

E       AssertionError: assert 1 == 0
E        +  where 1 = LintResult(exit_code=1, stdout='', stderr='No config file found, using default configuration\nTraceback (most recent c...t_obj = isort.SortImports(\nAttributeError: module \'isort\' has no attribute \'SortImports\'\n', linter_name='Pylint').exit_code
pants/backend/python/lint/pylint/rules_integration_test.py:181: AssertionError

Could be an instance of #10259. When will that land?

@Eric-Arellano
Copy link
Contributor

Landed a moment ago.

@tdyas tdyas force-pushed the default_remoting_to_streaming_client branch from 5cbe7fe to 499acb6 Compare July 6, 2020 05:03
@tdyas
Copy link
Contributor Author

tdyas commented Jul 6, 2020

slack conversation: https://pantsbuild.slack.com/archives/C0D7TNJHL/p1594053641453200

Gist: will remove polling client eventually

Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

@stuhood stuhood merged commit e7c4d0f into pantsbuild:master Jul 8, 2020
@tdyas tdyas deleted the default_remoting_to_streaming_client branch February 20, 2023 20:25
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.

None yet

3 participants