-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add getConnectionForSql and use it #988
Add getConnectionForSql and use it #988
Conversation
public static calculateMemberVersion(m: MemberVersion) : number { | ||
return BuildInfo.calculateServerVersion(m.major, m.minor, m.patch); | ||
} |
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.
This is added as a utility
this.config.connectionStrategy.asyncStart, | ||
this.config.connectionStrategy.reconnectMode, |
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.
refactored constructor to accept specific configs it needs
src/core/MemberVersion.ts
Outdated
|
||
/** | ||
* @param other other version to compare to | ||
* @return true if this version equals `other` | ||
*/ | ||
equals(other: MemberVersion): boolean { | ||
return this.major === other.major && this.minor === other.minor; | ||
} | ||
|
||
toString(): string { | ||
return `${this.major}.${this.minor}`; | ||
} |
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.
these are used in memberOfLargerSameVersionGroup
@@ -77,7 +77,7 @@ export class ClusterService implements Cluster { | |||
* @param uuid The UUID of the member as a string. | |||
* @return The member that was found, or undefined if not found. | |||
*/ | |||
getMember(uuid: string): MemberImpl { | |||
getMember(uuid: string): MemberImpl | undefined { |
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.
side change that is unrelated
private readonly asyncStart: boolean, | ||
private readonly reconnectMode: ReconnectMode, | ||
private readonly smartRoutingEnabled: boolean, | ||
private readonly loadBalancer: LoadBalancer, | ||
private readonly clusterService: ClusterService |
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.
preferred to use compact construction syntax(like other classes I refactored in #797 )
getRandomConnection(): Connection | null { | ||
if (this.smartRoutingEnabled) { | ||
let member; | ||
if (dataMember) { | ||
if (this.loadBalancer.canGetNextDataMember()) { | ||
member = this.loadBalancer.nextDataMember(); | ||
} else { | ||
member = null; | ||
const member = this.loadBalancer.next(); | ||
if (member != null) { | ||
const connection = this.getConnection(member.uuid); | ||
if (connection != null) { | ||
return connection; | ||
} | ||
} else { | ||
member = this.loadBalancer.next(); | ||
} | ||
} | ||
|
||
const iterator = this.activeConnections.values(); | ||
const next = iterator.next(); | ||
if (!next.done) { | ||
return next.value; | ||
} else { | ||
return null; | ||
} | ||
} |
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.
reverted to old version
const connection = this.getConnection(member.uuid); | ||
if (connection != null) { | ||
if (connection !== undefined) { |
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.
connection can only be undefined, I chose to be more specific here
@@ -255,7 +255,7 @@ export class SqlServiceImpl implements SqlService { | |||
* @param connection | |||
* @returns {@link HazelcastSqlException} | |||
*/ | |||
toHazelcastSqlException(err: any, connection: Connection) : HazelcastSqlException { | |||
rethrow(err: any, connection: Connection): HazelcastSqlException { |
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.
refactored so that rethrow takes err and connection and returns a HazelcastSqlException whereas toHazelcastSqlException is just for converting an err to HazelcastSqlException
Done this because I needed toHazelcastSqlException logic as a separate func
false, | ||
ReconnectMode.ON, |
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.
contructor change affected tests
@@ -231,6 +231,7 @@ describe('SqlResultTest', function () { | |||
beforeEach(function () { | |||
fakeSqlService = { | |||
toHazelcastSqlException: sandbox.fake((err) => new HazelcastSqlException(null, 1, '', err)), | |||
rethrow: sandbox.fake((err) => new HazelcastSqlException(null, 1, '', err)), |
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.
rethrow used in the sql service we need to add it too
|
||
it('should return data member connection when one exists and when data member is requested, [dummy mode]', | ||
function () { | ||
const firstUUID = UuidUtil.generate(); | ||
const secondUUID = UuidUtil.generate(); | ||
const thirdUUID = UuidUtil.generate(); | ||
|
||
const loadBalancerStub = {next: sandbox.spy(), nextDataMember: sandbox.spy()}; | ||
const clusterServiceStub = {}; | ||
clusterServiceStub.getMember = sandbox.stub(); | ||
|
||
clusterServiceStub.getMember.withArgs(firstUUID.toString()).returns({ | ||
liteMember: true | ||
}); | ||
clusterServiceStub.getMember.withArgs(secondUUID.toString()).returns({ | ||
liteMember: false | ||
}); | ||
clusterServiceStub.getMember.withArgs(thirdUUID.toString()).returns({ | ||
liteMember: true | ||
}); | ||
|
||
const connectionRegistry = new ConnectionRegistryImpl( | ||
new ConnectionStrategyConfigImpl(), | ||
false, | ||
loadBalancerStub, | ||
clusterServiceStub | ||
); | ||
|
||
const secondConnection = {}; | ||
const firstConnection = {}; | ||
connectionRegistry.setConnection(firstUUID, firstConnection); | ||
connectionRegistry.setConnection(secondUUID, secondConnection); | ||
connectionRegistry.setConnection(thirdUUID, {}); | ||
|
||
const connection = connectionRegistry.getRandomConnection(); | ||
const otherConnection = connectionRegistry.getRandomConnection(); | ||
const dataMemberConnection = connectionRegistry.getRandomConnection(true); | ||
|
||
expect(connection).to.be.equal(firstConnection); | ||
expect(otherConnection).to.be.equal(firstConnection); | ||
expect(dataMemberConnection).to.be.equal(secondConnection); | ||
|
||
expect(loadBalancerStub.next.called).to.be.false; | ||
expect(loadBalancerStub.nextDataMember.called).to.be.false; | ||
} | ||
); | ||
|
||
it('should return null if there is no data member connection and data member is requested, [dummy mode]', | ||
function () { | ||
const firstUUID = UuidUtil.generate(); | ||
const secondUUID = UuidUtil.generate(); | ||
|
||
const loadBalancerStub = {next: sandbox.spy(), nextDataMember: sandbox.spy()}; | ||
const clusterServiceStub = {}; | ||
clusterServiceStub.getMember = sandbox.stub(); | ||
|
||
clusterServiceStub.getMember.withArgs(firstUUID.toString()).returns({ | ||
liteMember: true | ||
}); | ||
clusterServiceStub.getMember.withArgs(secondUUID.toString()).returns({ | ||
liteMember: true | ||
}); | ||
|
||
const connectionRegistry = new ConnectionRegistryImpl( | ||
new ConnectionStrategyConfigImpl(), | ||
false, | ||
loadBalancerStub, | ||
clusterServiceStub | ||
); | ||
|
||
const secondConnection = {}; | ||
const firstConnection = {}; | ||
connectionRegistry.setConnection(firstUUID, firstConnection); | ||
connectionRegistry.setConnection(secondUUID, secondConnection); | ||
|
||
const connection = connectionRegistry.getRandomConnection(); | ||
const otherConnection = connectionRegistry.getRandomConnection(); | ||
const dataMemberConnection = connectionRegistry.getRandomConnection(true); | ||
|
||
expect(connection).to.be.equal(firstConnection); | ||
expect(otherConnection).to.be.equal(firstConnection); | ||
expect(dataMemberConnection).to.be.equal(null); | ||
|
||
expect(loadBalancerStub.next.called).to.be.false; | ||
expect(loadBalancerStub.nextDataMember.called).to.be.false; | ||
} | ||
); |
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.
deleted tests that are related to old get sql connection method(via getRandomConnection(dataMember))
Followup to #963. In 5.0 SQL team decided to use their own logic for selecting a connection for sending SQL message instead of using LoadBalancer.
Like java side did, this pr deprecates old methods on load balancer.