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

prefer secondary over slave referring to standby or secondary servers #1070

Merged
merged 6 commits into from Jan 15, 2018

Conversation

Projects
None yet
3 participants
@davecramer
Member

davecramer commented Jan 15, 2018

The community has decided that the naming of replicas in any form
should be referred to as secondary or standby.
We have chosen to use the word secondary
This PR renames (hopefully) all of the uses of slave to secondary

The community has decided that the naming of replicas in any form
should be referred to as secondary or standby.
We have chosen to use the word secondary
This PR renames (hopefully) all of the uses of slave to secondary
@@ -5,9 +5,13 @@
server=localhost
port=5432
secondaryServer=localhost
slaveServer=localhost

This comment has been minimized.

@vlsi

vlsi Jan 15, 2018

Member

should slave* be removed?

connectedHosts.add(getRemoteHostSpec());
tryConnectedHosts.addAll(hostStatusMap.keySet());
if (tryConnectedHosts.size() == 4) {
break;
}
}
assertEquals("Never connected to all salve hosts", new HashSet<String>(asList(slaveIp, slaveIp2)),
assertEquals("Never connected to all salve hosts", new HashSet<String>(asList(secondaryIP,

This comment has been minimized.

@vlsi

vlsi Jan 15, 2018

Member

salve -> secondary

@vlsi

This comment has been minimized.

Member

vlsi commented Jan 15, 2018

The missing bit is something like HostRequirement.valueOf that would automatically convert "slave" to "seconday", so the old spelling still works.

@vlsi

This comment has been minimized.

Member

vlsi commented Jan 15, 2018

I mean the code around

targetServerType = HostRequirement.valueOf(targetServerTypeStr);

@davecramer

This comment has been minimized.

Member

davecramer commented Jan 15, 2018

@vlsi

This comment has been minimized.

Member

vlsi commented Jan 15, 2018

Allow slave for one more major version then throw an error ?

Something like that should do. Giving the time it takes us to release a minor version, we would never have to deal with "removing the deprecated setting".

However, what if we still understand the value of "slave", yet just un-document it, and favour new values?

@codecov-io

This comment has been minimized.

codecov-io commented Jan 15, 2018

Codecov Report

Merging #1070 into master will decrease coverage by 0.01%.
The diff coverage is 73.33%.

@@             Coverage Diff              @@
##             master    #1070      +/-   ##
============================================
- Coverage     67.21%   67.19%   -0.02%     
+ Complexity     3660     3659       -1     
============================================
  Files           170      170              
  Lines         15615    15617       +2     
  Branches       2520     2520              
============================================
- Hits          10495    10494       -1     
- Misses         3935     3936       +1     
- Partials       1185     1187       +2
@davecramer

This comment has been minimized.

Member

davecramer commented Jan 15, 2018

Remove slave from build.properties
Modify HostRequirements so that we will accept slave or secondary for now
slave is deprecated but we will still accept it and not document it.
*/
public static HostRequirement getTargetServerType(String targetServerType) {
String allowSlave = targetServerType.replaceFirst("slave", targetServerType);

This comment has been minimized.

@vlsi

vlsi Jan 15, 2018

Member

I suggest targetServerType.replace('lave', 'econdary') to support both slave and preferSlave in one go.

@vlsi

This comment has been minimized.

Member

vlsi commented Jan 15, 2018

Looks good modulo javadoc getTargetServerType (I'm fine if the doc is completely removed from that method). Checkstyle warns on empty doc as well.

davecramer and others added some commits Jan 15, 2018

@davecramer davecramer merged commit 32c5390 into pgjdbc:master Jan 15, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

prefer secondary over slave referring to standby or secondary servers (
…pgjdbc#1070)

* The community has decided that the naming of replicas in any form
should be referred to as secondary or standby.
We have chosen to use the word secondary
This PR renames (hopefully) all of the uses of slave to secondary

* Remove slave from build.properties
Modify HostRequirements so that we will accept slave or secondary for now
slave is deprecated but we will still accept it and not document it.

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

prefer secondary over slave referring to standby or secondary servers (
…pgjdbc#1070)

* The community has decided that the naming of replicas in any form
should be referred to as secondary or standby.
We have chosen to use the word secondary
This PR renames (hopefully) all of the uses of slave to secondary

* Remove slave from build.properties
Modify HostRequirements so that we will accept slave or secondary for now
slave is deprecated but we will still accept it and not document it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment