-
Notifications
You must be signed in to change notification settings - Fork 25
feat(security): handle list mechanism response #136
Conversation
import javax.security.auth.Subject; | ||
import javax.security.sasl.Sasl; | ||
import org.slf4j.Logger; | ||
|
||
public class Negotiation { | ||
private static final Logger logger = org.slf4j.LoggerFactory.getLogger(Negotiation.class); | ||
// Because negotiation message is always the first rpc sent to pegasus server, | ||
// which will cost much more time. so we set negotiation timeout to 10s here | ||
private static final int negotiationTimeoutMS = 10000; |
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.
Negotiation should be set dynamically by the user timeout. It's under the user's control that how long an RPC should last.
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.
I don't think it must under the user's control. The negotiation process should be not visable to users
src/main/java/com/xiaomi/infra/pegasus/rpc/async/Negotiation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/async/SaslWrapper.java
Outdated
Show resolved
Hide resolved
src/test/java/com/xiaomi/infra/pegasus/rpc/async/NegotiationTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/async/SaslWrapper.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/async/Negotiation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/async/Negotiation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/async/Negotiation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/async/Negotiation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/async/Negotiation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/async/Negotiation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/interceptor/SecurityReplicaSessionInterceptor.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/async/ReplicaSession.java
Outdated
Show resolved
Hide resolved
src/test/java/com/xiaomi/infra/pegasus/rpc/async/NegotiationTest.java
Outdated
Show resolved
Hide resolved
…-client into on_recv_mechanisms
src/test/java/com/xiaomi/infra/pegasus/rpc/async/NegotiationTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/async/SaslWrapper.java
Outdated
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/interceptor/ReplicaSessionInterceptor.java
Show resolved
Hide resolved
src/main/java/com/xiaomi/infra/pegasus/rpc/interceptor/ReplicaSessionInterceptorManager.java
Outdated
Show resolved
Hide resolved
// return value: | ||
// true - pend succeed | ||
// false - pend failed | ||
public boolean tryPendRequest(RequestEntry entry) { | ||
// double check. the first one doesn't lock the lock. | ||
// Because negotiationSucceed only transfered from false to true. | ||
// So if it is true now, it will not change in the later. | ||
// But if it is false now, maybe it will change soon. So we should use lock to protect it. | ||
if (!this.negotiationSucceed) { | ||
synchronized (negotiationPendingSend) { | ||
if (!this.negotiationSucceed) { | ||
negotiationPendingSend.offer(entry); | ||
return true; | ||
} | ||
} | ||
} | ||
|
||
return 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.
-
Your design here binds the responsibility of negotiation to ReplicaSession again. Why couldn't ReplicaSessionInterceptor hold the pending queue?
-
negotiationPendingSend is declared final, but you offer items here.
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.
It's hard to hold pending queue by ReplicaSessionInterceptor
.
ReplicaSessionInterceptor
is a singleton, which will serves all of the ReplicaSession. So if we do this like you said, we must use a coarse-grained lock, which will produce low effiency- If we use
ReplicaSessionInterceptor
to do this, we must implement a lot of logic in it. For example timeout handling, which will mantain a timeout pendingResponse queue and a PendingSend, and so on. It's too complicated, and there is a lot of overlap code with ReplicaSession. And we should modify ReplicaSession to support it.
No description provided.