GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Remove gopher protocol support. by yoav-steinberg · Pull Request #9057 · redis/redis

Conversation

@yoav-steinberg

@yoav-steinberg yoav-steinberg commented Jun 8, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Gopher support was added mainly because it was simple (trivial to add).
But apparently even something that was trivial at the time, does cause complications
down the line when adding more features.
We recently ran into a few issues with io-threads conflicting with the gopher support.
We had to either complicate the code further in order to solve them, or drop gopher.
AFAIK it's completely unused, so we wanna chuck it, rather than keep supporting it.

See #8989

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
yoav-steinberg added 2 commits June 8, 2021 17:36
@yoav-steinberg
Remove gopher support. See redis#8989.
37d0426
@yoav-steinberg
Remove gopher from config file.
324886d
@yoav-steinberg yoav-steinberg added the approval-needed Waiting for core team approval to be merged label Jun 8, 2021
@yoav-steinberg
yoav-steinberg requested a review from oranagra June 8, 2021 14:56
@oranagra oranagra changed the title Remove gopher. See #8989. Remove gopher protocol support. Jun 9, 2021
@oranagra oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Jun 9, 2021
oranagra
oranagra approved these changes Jun 9, 2021
View reviewed changes

@oranagra oranagra left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@yoav-steinberg i added a top comment (to be used as commit comment), please review it and edit if necessary.

@redis/core-team please approve.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/config.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
madolson
madolson approved these changes Jun 11, 2021
View reviewed changes
@yoav-steinberg

yoav-steinberg commented Jun 15, 2021

Copy link
Copy Markdown
Contributor Author

@oranagra The top comment is good. Fixed some typos. You can merge.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
yossigo
yossigo approved these changes Jun 15, 2021
View reviewed changes
@oranagra
oranagra merged commit 362786c into redis:unstable Jun 16, 2021
@oranagra oranagra mentioned this pull request Jun 16, 2021
Adds test for gopher prtocol support. #8989
Closed
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
@yoav-steinberg
Remove gopher protocol support. (redis#9057)
6042eab 
Gopher support was added mainly because it was simple (trivial to add).
But apparently even something that was trivial at the time, does cause complications
down the line when adding more features.
We recently ran into a few issues with io-threads conflicting with the gopher support.
We had to either complicate the code further in order to solve them, or drop gopher.
AFAIK it's completely unused, so we wanna chuck it, rather than keep supporting it.
@oranagra oranagra added the breaking-change This change can potentially break existing application label Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes
+2 more reviewers
@yossigo yossigo yossigo approved these changes
@madolson madolson madolson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

approval-needed Waiting for core team approval to be merged breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Redis 7.0
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@yoav-steinberg @yossigo @oranagra @madolson
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.