Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Commit

Permalink
rhbz988202 - remove rate limiter from REST limiter
Browse files Browse the repository at this point in the history
  • Loading branch information
Patrick Huang committed Mar 28, 2014
1 parent e94c1f4 commit 560c794
Show file tree
Hide file tree
Showing 13 changed files with 71 additions and 182 deletions.
13 changes: 0 additions & 13 deletions functional-test/src/main/java/org/zanata/page/administration/ServerConfigurationPage.java
View file
Expand Up @@ -15,9 +15,6 @@ public class ServerConfigurationPage extends BasePage {
@FindBy(id = "serverConfigForm:urlField")
private WebElement urlField;

@FindBy(id = "serverConfigForm:rateLimitField:rateLimitEml")
private WebElement rateLimitField;

@FindBy(
id = "serverConfigForm:maxConcurrentPerApiKeyField:maxConcurrentPerApiKeyEml")
private WebElement maxConcurrentField;
Expand All @@ -33,16 +30,6 @@ public ServerConfigurationPage(WebDriver driver) {
super(driver);
}

public ServerConfigurationPage inputRateLimit(int limit) {
rateLimitField.clear();
rateLimitField.sendKeys(limit + "");
return this;
}

public String getRateLimit() {
return rateLimitField.getAttribute("value");
}

public ServerConfigurationPage inputMaxConcurrent(int max) {
maxConcurrentField.clear();
maxConcurrentField.sendKeys(max + "");
Expand Down
105 changes: 58 additions & 47 deletions functional-test/src/test/java/org/zanata/feature/misc/RateLimitRestAndUITest.java
View file
Expand Up @@ -17,7 +17,6 @@
import org.junit.Test;
import org.junit.experimental.categories.Category;
import org.zanata.feature.DetailedTest;
import org.zanata.model.HApplicationConfiguration;
import org.zanata.page.administration.AdministrationPage;
import org.zanata.page.administration.ServerConfigurationPage;
import org.zanata.util.AddUsersRule;
Expand All @@ -37,7 +36,6 @@
import static org.zanata.model.HApplicationConfiguration.KEY_ADMIN_EMAIL;
import static org.zanata.model.HApplicationConfiguration.KEY_MAX_ACTIVE_REQ_PER_API_KEY;
import static org.zanata.model.HApplicationConfiguration.KEY_MAX_CONCURRENT_REQ_PER_API_KEY;
import static org.zanata.model.HApplicationConfiguration.KEY_RATE_LIMIT_PER_SECOND;
import static org.zanata.util.ZanataRestCaller.checkStatusAndReleaseConnection;
import static org.zanata.util.ZanataRestCaller.getStatusAndReleaseConnection;

Expand All @@ -52,14 +50,14 @@ public class RateLimitRestAndUITest {
public AddUsersRule addUsersRule = new AddUsersRule();

// because of the time based nature, tests may fail occasionally
// @Rule
// @Rule
public RetryRule retryRule = new RetryRule(2);

private static final String TRANSLATOR = "translator";
private static final String TRANSLATOR_API =
"d83882201764f7d339e97c4b087f0806";
private String rateLimitingPathParam = "c/" + KEY_RATE_LIMIT_PER_SECOND;
private String maxConcurrentPathParam = "c/" + KEY_MAX_CONCURRENT_REQ_PER_API_KEY;
private String maxConcurrentPathParam = "c/"
+ KEY_MAX_CONCURRENT_REQ_PER_API_KEY;
private String maxActivePathParam = "c/" + KEY_MAX_ACTIVE_REQ_PER_API_KEY;

@Test
Expand All @@ -70,48 +68,53 @@ public void canConfigureRateLimitByWebUI() {
basicWorkFlow.goToPage("admin/server_configuration",
ServerConfigurationPage.class);

assertThat(serverConfigPage.getRateLimit(), Matchers.isEmptyString());
assertThat(serverConfigPage.getMaxConcurrentRequestsPerApiKey(), Matchers.isEmptyString());
assertThat(serverConfigPage.getMaxActiveRequestsPerApiKey(), Matchers.isEmptyString());
assertThat(serverConfigPage.getMaxConcurrentRequestsPerApiKey(),
Matchers.isEmptyString());
assertThat(serverConfigPage.getMaxActiveRequestsPerApiKey(),
Matchers.isEmptyString());

AdministrationPage administrationPage =
serverConfigPage.inputRateLimit(1).inputMaxConcurrent(5)
.inputMaxActive(3).save();
serverConfigPage.inputMaxConcurrent(5).inputMaxActive(3).save();

assertThat(administrationPage.getNotificationMessage(),
Matchers.equalTo("Configuration was successfully updated."));

serverConfigPage =
basicWorkFlow.goToPage("admin/server_configuration",
ServerConfigurationPage.class);
assertThat(serverConfigPage.getRateLimit(), Matchers.equalTo("1"));
assertThat(serverConfigPage.getMaxActiveRequestsPerApiKey(),
Matchers.equalTo("3"));
assertThat(serverConfigPage.getMaxConcurrentRequestsPerApiKey(),
Matchers.equalTo("5"));
}

@Test
public void canCallServerConfigurationRestService() throws Exception {
ClientRequest clientRequest = clientRequestAsAdmin(
"rest/configurations/" + rateLimitingPathParam);
ClientRequest clientRequest =
clientRequestAsAdmin("rest/configurations/"
+ maxConcurrentPathParam);
clientRequest.body("text/plain", "1");
// can put
Response putResponse = clientRequest.put();

assertThat(getStatusAndReleaseConnection(putResponse), Matchers.is(201));

// can get single configuration
Response getResponse = clientRequestAsAdmin(
"rest/configurations/" + rateLimitingPathParam).get();
Response getResponse =
clientRequestAsAdmin(
"rest/configurations/" + maxConcurrentPathParam).get();

assertThat(getResponse.getStatus(), Matchers.is(200));
String rateLimitConfig =
((BaseClientResponse<String>) getResponse)
.getEntity(String.class);
assertThat(rateLimitConfig,
Matchers.containsString(KEY_RATE_LIMIT_PER_SECOND));
Matchers.containsString(KEY_MAX_CONCURRENT_REQ_PER_API_KEY));
assertThat(rateLimitConfig, Matchers.containsString("<value>1</value>"));

// can get all configurations
Response getAllResponse = clientRequestAsAdmin(
"rest/configurations/").get();
Response getAllResponse =
clientRequestAsAdmin("rest/configurations/").get();
BaseClientResponse<String> baseClientResponse =
(BaseClientResponse) getAllResponse;

Expand All @@ -124,11 +127,13 @@ public void canCallServerConfigurationRestService() throws Exception {
}

private static ClientRequest clientRequestAsAdmin(String path) {
ClientRequest clientRequest = new ClientRequest(
PropertiesHolder.getProperty(Constants.zanataInstance.value()) +
path);
ClientRequest clientRequest =
new ClientRequest(
PropertiesHolder.getProperty(Constants.zanataInstance
.value()) + path);
clientRequest.header("X-Auth-User", "admin");
clientRequest.header("X-Auth-Token", PropertiesHolder.getProperty(Constants.zanataApiKey.value()));
clientRequest.header("X-Auth-Token",
PropertiesHolder.getProperty(Constants.zanataApiKey.value()));
clientRequest.header("Content-Type", "application/xml");
return clientRequest;
}
Expand All @@ -155,9 +160,10 @@ public void serverConfigurationRestServiceOnlyAvailableToAdmin()
}

private static ClientRequest clientRequestAsTranslator(String path) {
ClientRequest clientRequest = new ClientRequest(
PropertiesHolder.getProperty(Constants.zanataInstance.value()) +
path);
ClientRequest clientRequest =
new ClientRequest(
PropertiesHolder.getProperty(Constants.zanataInstance
.value()) + path);
clientRequest.header("X-Auth-User", TRANSLATOR);
clientRequest.header("X-Auth-Token", TRANSLATOR_API);
clientRequest.header("Content-Type", "application/xml");
Expand All @@ -166,28 +172,28 @@ private static ClientRequest clientRequestAsTranslator(String path) {

@Test
public void canOnlyDealWithKnownConfiguration() throws Exception {
ClientRequest clientRequest = clientRequestAsAdmin(
"rest/configurations/c/abc");
ClientRequest clientRequest =
clientRequestAsAdmin("rest/configurations/c/abc");

Response putResponse = clientRequest.put();
assertThat(getStatusAndReleaseConnection(putResponse), Matchers.is(400));

Response getResponse = clientRequestAsAdmin(
"rest/configurations/c/abc").get();
Response getResponse =
clientRequestAsAdmin("rest/configurations/c/abc").get();
assertThat(getStatusAndReleaseConnection(getResponse), Matchers.is(404));
}

@Test
public void canLimitConcurrentRestRequestsPerAPIKey() throws Exception {
// translator creates the project/version
// translator creates the project/version
final String projectSlug = "project";
final String iterationSlug = "version";
new ZanataRestCaller(TRANSLATOR, TRANSLATOR_API)
.createProjectAndVersion(projectSlug, iterationSlug, "gettext");


ClientRequest clientRequest = clientRequestAsAdmin(
"rest/configurations/" + maxConcurrentPathParam);
ClientRequest clientRequest =
clientRequestAsAdmin("rest/configurations/"
+ maxConcurrentPathParam);
clientRequest.body(MediaType.TEXT_PLAIN_TYPE, "2");

checkStatusAndReleaseConnection(clientRequest.put());
Expand Down Expand Up @@ -235,47 +241,50 @@ ImmutableList.<Callable<Integer>> builder()

// 1 request from translator should get 403 and fail
log.info("result: {}", result);
assertThat(result,
Matchers.containsInAnyOrder(201, 201, 201, 201, 429));
assertThat(result, Matchers.containsInAnyOrder(201, 201, 201, 201, 429));
}

@Test(timeout = 5000)
public void exceptionWillReleaseSemaphore() throws Exception {
// Given: max active is set to 1
ClientRequest configRequest = clientRequestAsAdmin(
"rest/configurations/" + maxActivePathParam);
ClientRequest configRequest =
clientRequestAsAdmin("rest/configurations/"
+ maxActivePathParam);
configRequest.body(MediaType.TEXT_PLAIN_TYPE, "1");
checkStatusAndReleaseConnection(configRequest.put());

// When: multiple requests that will result in a mapped exception
ClientRequest clientRequest = clientRequestAsAdmin(
"rest/test/data/sample/dummy?exception=org.zanata.rest.NoSuchEntityException");
ClientRequest clientRequest =
clientRequestAsAdmin("rest/test/data/sample/dummy?exception=org.zanata.rest.NoSuchEntityException");
getStatusAndReleaseConnection(clientRequest.get());
getStatusAndReleaseConnection(clientRequest.get());
getStatusAndReleaseConnection(clientRequest.get());
getStatusAndReleaseConnection(clientRequest.get());

// Then: request that result in exception should still release semaphore. i.e. no permit leak
// Then: request that result in exception should still release
// semaphore. i.e. no permit leak
assertThat(1, Matchers.is(1));
}

@Test(timeout = 5000)
public void unmappedExceptionWillAlsoReleaseSemaphore() throws Exception {
// Given: max active is set to 1
ClientRequest configRequest = clientRequestAsAdmin(
"rest/configurations/" + maxActivePathParam);
ClientRequest configRequest =
clientRequestAsAdmin("rest/configurations/"
+ maxActivePathParam);
configRequest.body(MediaType.TEXT_PLAIN_TYPE, "1");
checkStatusAndReleaseConnection(configRequest.put());

// When: multiple requests that will result in an unmapped exception
ClientRequest clientRequest = clientRequestAsAdmin(
"rest/test/data/sample/dummy?exception=java.lang.RuntimeException");
ClientRequest clientRequest =
clientRequestAsAdmin("rest/test/data/sample/dummy?exception=java.lang.RuntimeException");
getStatusAndReleaseConnection(clientRequest.get());
getStatusAndReleaseConnection(clientRequest.get());
getStatusAndReleaseConnection(clientRequest.get());
getStatusAndReleaseConnection(clientRequest.get());

// Then: request that result in exception should still release semaphore. i.e. no permit leak
// Then: request that result in exception should still release
// semaphore. i.e. no permit leak
assertThat(1, Matchers.is(1));
}

Expand Down Expand Up @@ -303,9 +312,11 @@ public Integer apply(Future<Integer> input) {
try {
return input.get();
} catch (Exception e) {
// by using filter we lose RESTeasy's exception translation
// by using filter we lose RESTeasy's exception
// translation
String message = e.getMessage().toLowerCase();
if (message.matches(".+429.+too many concurrent request.+")) {
if (message
.matches(".+429.+too many concurrent request.+")) {
return 429;
}
throw Throwables.propagate(e);
Expand Down
1 change: 0 additions & 1 deletion zanata-model/src/main/java/org/zanata/model/HApplicationConfiguration.java
View file
Expand Up @@ -57,7 +57,6 @@ public class HApplicationConfiguration extends ModelEntityBase {
public static String KEY_PIWIK_URL = "piwik.url";
public static String KEY_PIWIK_IDSITE = "piwik.idSite";
public static String KEY_TERMS_CONDITIONS_URL = "terms.conditions.url";
public static String KEY_RATE_LIMIT_PER_SECOND = "rate.limit.per.second.per.apikey";
public static String KEY_MAX_CONCURRENT_REQ_PER_API_KEY = "max.concurrent.req.per.apikey";
public static String KEY_MAX_ACTIVE_REQ_PER_API_KEY = "max.active.req.per.apikey";

Expand Down
10 changes: 0 additions & 10 deletions zanata-war/src/main/java/org/zanata/ApplicationConfiguration.java
View file
Expand Up @@ -392,16 +392,6 @@ private String getBaseWebAssetsUrl() {
"//assets-zanata.rhcloud.com");
}

public double getRateLimitPerSecond() {
String limitPerSecond =
databaseBackedConfig.getRateLimitPerSecond();
if (Strings.isNullOrEmpty(limitPerSecond)) {
// 0 means no limit
return 0;
}
return Double.parseDouble(limitPerSecond);
}

public int getMaxConcurrentRequestsPerApiKey() {
String max =
databaseBackedConfig.getMaxConcurrentRequestsPerApiKey();
Expand Down
16 changes: 0 additions & 16 deletions zanata-war/src/main/java/org/zanata/action/ServerConfigurationBean.java
View file
Expand Up @@ -98,9 +98,6 @@ public class ServerConfigurationBean implements Serializable {
@Url(canEndInSlash = true)
private String termsOfUseUrl;

@Pattern(regexp = "\\d{0,5}")
private String rateLimitPerSecond;

@Pattern(regexp = "\\d{0,5}")
private String maxConcurrentRequestsPerApiKey;

Expand Down Expand Up @@ -217,12 +214,6 @@ public void onCreate() {
this.termsOfUseUrl = termsOfUseUrlValue.getValue();
}

HApplicationConfiguration rateLimitValue = applicationConfigurationDAO
.findByKey(HApplicationConfiguration.KEY_RATE_LIMIT_PER_SECOND);
if (rateLimitValue != null) {
this.rateLimitPerSecond = rateLimitValue.getValue();
}

HApplicationConfiguration maxConcurrent = applicationConfigurationDAO.findByKey(
HApplicationConfiguration.KEY_MAX_CONCURRENT_REQ_PER_API_KEY);
if (maxConcurrent != null) {
Expand Down Expand Up @@ -324,13 +315,6 @@ public String update() {
HApplicationConfiguration.KEY_TERMS_CONDITIONS_URL,
termsOfUseUrlValue, termsOfUseUrl, applicationConfigurationDAO);

HApplicationConfiguration rateLimitValue =
applicationConfigurationDAO
.findByKey(HApplicationConfiguration.KEY_RATE_LIMIT_PER_SECOND);
ServerConfigurationService.persistApplicationConfig(
HApplicationConfiguration.KEY_RATE_LIMIT_PER_SECOND,
rateLimitValue, rateLimitPerSecond, applicationConfigurationDAO);

HApplicationConfiguration maxConcurrent =
applicationConfigurationDAO
.findByKey(
Expand Down
4 changes: 0 additions & 4 deletions zanata-war/src/main/java/org/zanata/config/DatabaseBackedConfig.java
View file
Expand Up @@ -158,10 +158,6 @@ public String getTermsOfUseUrl() {
return getConfigValue(HApplicationConfiguration.KEY_TERMS_CONDITIONS_URL);
}

public String getRateLimitPerSecond() {
return getConfigValue(HApplicationConfiguration.KEY_RATE_LIMIT_PER_SECOND);
}

public String getMaxConcurrentRequestsPerApiKey() {
return getConfigValue(HApplicationConfiguration.KEY_MAX_CONCURRENT_REQ_PER_API_KEY);
}
Expand Down
4 changes: 1 addition & 3 deletions zanata-war/src/main/java/org/zanata/limits/RateLimitManager.java
View file
Expand Up @@ -58,10 +58,8 @@ private void readRateLimitState() {
.getInstance("applicationConfiguration");
int maxConcurrent = appConfig.getMaxConcurrentRequestsPerApiKey();
int maxActive = appConfig.getMaxActiveRequestsPerApiKey();
double rateLimitPerSecond = appConfig.getRateLimitPerSecond();
limitConfig =
new RestCallLimiter.RateLimitConfig(maxConcurrent, maxActive,
rateLimitPerSecond);
new RestCallLimiter.RateLimitConfig(maxConcurrent, maxActive);
}

@Observer({ ApplicationConfiguration.EVENT_CONFIGURATION_CHANGED })
Expand Down
3 changes: 1 addition & 2 deletions zanata-war/src/main/java/org/zanata/limits/RateLimitingProcessor.java
View file
Expand Up @@ -43,8 +43,7 @@ public LeakyBucket apply(String input) {
ApplicationConfiguration appConfig = getApplicationConfiguration();

if (appConfig.getMaxConcurrentRequestsPerApiKey() == 0
&& appConfig.getMaxActiveRequestsPerApiKey() == 0
&& appConfig.getRateLimitPerSecond() == 0) {
&& appConfig.getMaxActiveRequestsPerApiKey() == 0) {
// short circuit if we don't want limiting
taskToRun.run();
return;
Expand Down

0 comments on commit 560c794

Please sign in to comment.