-
Notifications
You must be signed in to change notification settings - Fork 14
Implement rotating write endpoints for MemqProducer #45
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
Conversation
…rioritization + removals after multiple failures
vahidhashemian
left a comment
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.
Thanks! Left a few comments. Also, would be great to add:
- Javadoc for the new code/methods/configs
- metrics where it makes sense
memq-client/src/main/java/com/pinterest/memq/client/commons2/MemqCommonClient.java
Show resolved
Hide resolved
memq-client/src/main/java/com/pinterest/memq/client/commons2/MemqCommonClient.java
Show resolved
Hide resolved
memq-client/src/main/java/com/pinterest/memq/client/commons2/network/NetworkClient.java
Show resolved
Hide resolved
memq-client/src/main/java/com/pinterest/memq/client/commons2/network/NetworkClient.java
Show resolved
Hide resolved
memq-client/src/main/java/com/pinterest/memq/client/commons2/MemqCommonClient.java
Outdated
Show resolved
Hide resolved
memq-client/src/main/java/com/pinterest/memq/client/commons2/MemqCommonClient.java
Show resolved
Hide resolved
memq-client/src/main/java/com/pinterest/memq/client/commons2/MemqCommonClient.java
Outdated
Show resolved
Hide resolved
memq-client/src/main/java/com/pinterest/memq/client/commons2/MemqCommonClient.java
Show resolved
Hide resolved
memq-client/src/main/java/com/pinterest/memq/client/commons2/MemqCommonClient.java
Outdated
Show resolved
Hide resolved
memq-commons/src/main/java/com/pinterest/memq/commons/protocol/Broker.java
Show resolved
Hide resolved
| deprioritizeDeadEndpoint(endpoint, topic); // this endpoint is down even after retries in NetworkClient, remove it from the write endpoints and take another one from locality endpoints | ||
| } catch (Exception ex) { | ||
| logger.error("Failed to refresh write endpoints", ex); | ||
| throw e; |
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.
Should it throw ex too?
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.
we're throwing e instead of ex because ex is a secondary error thrown by deprioritizeDeadEndpoint(). the main issue is the ConnectException.
| if (currentWrites.size() < numWriteEndpoints && !currentWrites.contains(endpoint)) { | ||
| logger.info("Registering write endpoint: " + endpoint + " for topic: " + topic); | ||
| writeEndpoints.add(endpoint); | ||
| List<Endpoint> newWrites = new ArrayList<>(currentWrites); | ||
| newWrites.add(endpoint); | ||
| this.writeEndpoints = Collections.unmodifiableList(newWrites); | ||
| } |
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.
Is it still possible for two threads to overwrite each other in this if block?
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.
hypothetically yes, but the caller of MemqCommonClient is the Request.Dispatch thread which is single-threaded: https://github.com/pinterest/memq/blob/main/memq-client/src/main/java/com/pinterest/memq/client/producer2/RequestManager.java#L84
Allow MemQ producers to maintain a sticky set of N brokers for round-robin writes. This probabilistically reduces the variance in broker throughput vs. just one sticky connection.
The idea is to maintain the favorable properties of the current endpoint selection algorithm but expand it to accommodate N endpoints in rotation. The favorable properties we want to maintain are:
To accommodate N endpoints in rotation, we only slightly modify the original endpoint selection algorithm so that we replace the singleton currentEndpoint and instead keep a set of writeEndpoints which has a maximum size of numWriteEndpoints. The modified algorithm looks similar to the existing algorithm:
Changes are made to the following core classes:
Map<InetSocketAddress, ChannelFuture> channelPoolto maintain a set of open channels for each endpointchannelToRequestsandrequestsToChannelto track and maintain inflight requests for multiple channelsTesting: