Open source memq actions #272
Conversation
orion-commons/src/main/java/com/pinterest/orion/utils/EC2Helper.java
Outdated
Show resolved
Hide resolved
| return METHOD_NAME; | ||
| } | ||
|
|
||
| public HttpDeleteWithBody(final String uri) { |
There was a problem hiding this comment.
Is the uri going to be the body of the request?
I'm confused by the class definition claiming to allow a body in the request, but not providing any additional interfaces to support that.
There was a problem hiding this comment.
No. That's the target URI of this HTTP request.
For regular HttpDelete request, I cannot run setEntity and attach the instance id to it.
With this HttpDeleteWithBody class, it will be executed as HttpDelete request by the http client because of the getMethod value, and extends HttpEntityEnclosingRequestBase allows me to attach body (entity) to it.
orion-commons/src/main/java/com/pinterest/orion/utils/TeletraanClient.java
Outdated
Show resolved
Hide resolved
orion-commons/src/main/java/com/pinterest/orion/utils/TeletraanClient.java
Outdated
Show resolved
Hide resolved
orion-commons/src/main/java/com/pinterest/orion/utils/TeletraanClient.java
Outdated
Show resolved
Hide resolved
orion-commons/src/main/java/com/pinterest/orion/utils/TeletraanClient.java
Outdated
Show resolved
Hide resolved
| * @param overrideTeletraanToken | ||
| * @return boolean indicating if the instance is pending termination | ||
| */ | ||
| public boolean isInstancePendingTermination( |
There was a problem hiding this comment.
Why not have one function for this status check? There was another one above.
There was a problem hiding this comment.
Which one? isHostPendingTermination is getting the status and parse the response to know the host status. isHostTerminated is checking the host cannot be found. They cannot be merged together. The shared part (HTTP call) is in getHostStatusHistory
There was a problem hiding this comment.
The reason I asked is both functions return a boolean.
There was a problem hiding this comment.
One is for parsing http entity to get the result, another one is for to make http call itself. Any suggestion for method names? I use isInstancePendingTermination/IsInstancePendingTermination as you suggested.
| import com.pinterest.orion.core.actions.Action; | ||
| import com.pinterest.orion.core.actions.generic.GenericClusterWideAction; | ||
|
|
||
| public abstract class MemqClusterDecommissionBrokers extends GenericClusterWideAction.ParallelDecommissionNodeAction { |
There was a problem hiding this comment.
This abstract class is not used anywhere; do we need a basic implementation of it? Same for below two classes.
There was a problem hiding this comment.
This class will be inheritant to run multiple MemqBrokerDecommissionActions in parallel (ClusterDecomission->BrokerDecomission). I will add more details in javadoc.
| import org.apache.http.impl.client.CloseableHttpClient; | ||
| import org.apache.http.impl.client.HttpClientBuilder; | ||
|
|
||
| public abstract class MemqTeletraanBrokerReplacementAction extends NodeAction { |
There was a problem hiding this comment.
Is it possible to combine replacement and decommissioning, or make one dependent on the other? I feel the termination part should be the same between the two.
Or this change has to be done at the parent side?
There was a problem hiding this comment.
I tried to merge the common parts of these two classes into one parent class. It looks ugly. The wait check looks similar but the APIs are different. Plus there're extra steps in replacement workflow. And the existing parent classes are also different. So I rather keep them in two classes and inherant from different parent classes, following existing kafka oop structure.
| import java.net.HttpURLConnection; | ||
| import java.util.logging.Logger; | ||
|
|
||
| public class TeletraanClient { |
There was a problem hiding this comment.
Should we introduce an interface for instance operations (if there is not one already)? With this interface, we can have Kafka's way of instance operations and Teletraan based operations be implementations of that interface. Then each user will be able to pick which option to choose for which type of system.
There was a problem hiding this comment.
Make sense. I will include the operations in EC2 helper as well
| /** | ||
| * Constants for Teletraan client | ||
| */ | ||
| public class TeletraanConstants { |
There was a problem hiding this comment.
nit: Change to Constants since teletraan is already in the package?
There was a problem hiding this comment.
Make sense. Changed
| * @param region the region of the host | ||
| * @return the instance id | ||
| */ | ||
| public abstract String getHostIdUsingHostName(String fullHostName, String region); |
There was a problem hiding this comment.
Sorry for bringing this up again. The Java doc says the function gets instance id but the name has host id. If host id it the same as EC2 instance id, I suggest using the term instance id as it's more commonly used and not confusing.
There was a problem hiding this comment.
Make sense. Changed
Open source orion memq broker actions: reboot / replace / decommission
The PR includes 3 memq actions
And a helper classes to achieve thos actions
Add teletraan client, which is a wrapper of teletraan http api call.
It also includes the parallel action abstract class. They can be used to run the above actions in parallel.
To use the actions, the internal package needs to override the EC2 helper in the main classes. For details, there's a readme.