You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@omid.apache.org by daijyc <gi...@git.apache.org> on 2016/08/08 06:49:06 UTC
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
GitHub user daijyc opened a pull request:
https://github.com/apache/incubator-omid/pull/3
OMID-50: Provide an option to reduce tso-server CPU usage
A revision based on Francisco's comments.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/daijyc/incubator-omid OMID-50
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-omid/pull/3.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3
----
commit 93be1325c1d9fe9f3f340e55d6031b1a37f454d2
Author: Daniel Dai <da...@hortonworks.com>
Date: 2016-08-08T06:09:07Z
OMID-50: Provide an option to reduce tso-server CPU usage
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by francisco-perez-sorrosal <gi...@git.apache.org>.
Github user francisco-perez-sorrosal commented on a diff in the pull request:
https://github.com/apache/incubator-omid/pull/3#discussion_r73977412
--- Diff: tso-server/src/main/java/org/apache/omid/tso/RetryProcessorImpl.java ---
@@ -69,7 +72,8 @@
private final Meter noCTFoundMeter;
@Inject
- RetryProcessorImpl(MetricsRegistry metrics,
+ RetryProcessorImpl(TSOServerConfig config,
+ MetricsRegistry metrics,
--- End diff --
Same comment as above. Inject strategy instead of tso config:
```java
@Inject
RetryProcessorImpl(@Named("RetryStrategy") WaitStrategy strategy,
MetricsRegistry metrics,
CommitTable commitTable,
ReplyProcessor replyProc,
Panicker panicker,
ObjectPool<Batch> batchPool)
throws InterruptedException, ExecutionException, IOException {
// ------------------------------------------------------------------------------------------------------------
// Disruptor initialization
// ------------------------------------------------------------------------------------------------------------
ThreadFactory threadFactory = new ThreadFactoryBuilder().setNameFormat("retry-%d").build();
this.disruptorExec = Executors.newSingleThreadExecutor(threadFactory);
this.disruptor = new Disruptor<>(EVENT_FACTORY, 1 << 12, disruptorExec, SINGLE, strategy);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by francisco-perez-sorrosal <gi...@git.apache.org>.
Github user francisco-perez-sorrosal commented on a diff in the pull request:
https://github.com/apache/incubator-omid/pull/3#discussion_r73977302
--- Diff: tso-server/src/main/java/org/apache/omid/tso/DisruptorModule.java ---
@@ -18,14 +18,38 @@
package org.apache.omid.tso;
import com.google.inject.AbstractModule;
+import com.google.inject.name.Names;
+import com.lmax.disruptor.BlockingWaitStrategy;
+import com.lmax.disruptor.BusySpinWaitStrategy;
+import com.lmax.disruptor.WaitStrategy;
import javax.inject.Singleton;
public class DisruptorModule extends AbstractModule {
+ private final TSOServerConfig config;
+
+ public DisruptorModule(TSOServerConfig config) {
+ this.config = config;
+ }
+
@Override
protected void configure() {
-
+ switch (config.getWaitStrategyEnum()) {
+ // A low-cpu usage Disruptor configuration for using in local/test environments
+ case LOW_CPU:
+ bind(WaitStrategy.class).annotatedWith(Names.named("PersistenceStrategy")).to(BlockingWaitStrategy.class);
+ bind(WaitStrategy.class).annotatedWith(Names.named("ReplyStrategy")).to(BlockingWaitStrategy.class);
+ bind(WaitStrategy.class).annotatedWith(Names.named("RetryStrategy")).to(BlockingWaitStrategy.class);
--- End diff --
I've realized that this should be a `YieldingWaitStrategy` according to the current code (see below). So this should be:
```
bind(WaitStrategy.class).annotatedWith(Names.named("RetryStrategy")).to(YieldingWaitStrategy.class);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by francisco-perez-sorrosal <gi...@git.apache.org>.
Github user francisco-perez-sorrosal commented on a diff in the pull request:
https://github.com/apache/incubator-omid/pull/3#discussion_r74187443
--- Diff: tso-server/src/main/java/org/apache/omid/tso/RetryProcessorImpl.java ---
@@ -69,7 +72,8 @@
private final Meter noCTFoundMeter;
@Inject
- RetryProcessorImpl(MetricsRegistry metrics,
+ RetryProcessorImpl(@Named("PersistenceStrategy") WaitStrategy strategy,
--- End diff --
Typo in named. Should be the same name that is in the DisruptorModule.java for the retry processor:
```
@Named("RetryStrategy")
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-omid/pull/3
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by francisco-perez-sorrosal <gi...@git.apache.org>.
Github user francisco-perez-sorrosal commented on a diff in the pull request:
https://github.com/apache/incubator-omid/pull/3#discussion_r73977426
--- Diff: tso-server/src/main/java/org/apache/omid/tso/TSOServerConfig.java ---
@@ -154,4 +161,16 @@ public void setMetrics(MetricsRegistry metrics) {
this.metrics = metrics;
}
+ public String getWaitStrategy() {
+ return waitStrategy;
+ }
+
+ public WAIT_STRATEGY getWaitStrategyEnum() {
+ return TSOServerConfig.WAIT_STRATEGY.valueOf(waitStrategy);
+ }
+
+
--- End diff --
Typo: Remove one empty line
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid issue #3: OMID-50: Provide an option to reduce tso-server CPU...
Posted by daijyc <gi...@git.apache.org>.
Github user daijyc commented on the issue:
https://github.com/apache/incubator-omid/pull/3
Yes, I apparently miss something in the patch. Thanks for pointing out, and those should be fixed now.
I did a "commit --amend" and then "push -force". The PR get updated. Probably I shall not do that, instead push the delta. Let me know if that's the case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by francisco-perez-sorrosal <gi...@git.apache.org>.
Github user francisco-perez-sorrosal commented on a diff in the pull request:
https://github.com/apache/incubator-omid/pull/3#discussion_r73977366
--- Diff: tso-server/src/main/java/org/apache/omid/tso/PersistenceProcessorImpl.java ---
@@ -83,7 +86,9 @@
ThreadFactoryBuilder threadFactory = new ThreadFactoryBuilder().setNameFormat("persist-%d");
this.disruptorExec = Executors.newFixedThreadPool(config.getNumConcurrentCTWriters(), threadFactory.build());
- this.disruptor = new Disruptor<>(EVENT_FACTORY, 1 << 20, disruptorExec , SINGLE, new BusySpinWaitStrategy());
+ this.disruptor = new Disruptor<>(EVENT_FACTORY, 1 << 20, disruptorExec , SINGLE,
--- End diff --
I think you forgot to add the first part of my comment. Instead of creating each strategy explicitly in each *ProcessorImpl class, we inject the WaitStrategy in it (that's why we add it to the `DisruptorModule` above.) So for this class should be:
```java
@Inject
PersistenceProcessorImpl(TSOServerConfig config,
@Named("PersistenceStrategy") WaitStrategy strategy, // <-- Strategy is injected here
CommitTable commitTable,
ObjectPool<Batch> batchPool,
Panicker panicker,
PersistenceProcessorHandler[] handlers,
MetricsRegistry metrics)
throws Exception {
// ------------------------------------------------------------------------------------------------------------
// Disruptor initialization
// ------------------------------------------------------------------------------------------------------------
ThreadFactoryBuilder threadFactory = new ThreadFactoryBuilder().setNameFormat("persist-%d");
this.disruptorExec = Executors.newFixedThreadPool(config.getNumConcurrentCTWriters(), threadFactory.build());
this.disruptor = new Disruptor<>(EVENT_FACTORY, 1 << 20, disruptorExec , SINGLE, strategy); // <-- Use injected strategy here
...
```
Please remove my comments, if you cut and paste.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by francisco-perez-sorrosal <gi...@git.apache.org>.
Github user francisco-perez-sorrosal commented on a diff in the pull request:
https://github.com/apache/incubator-omid/pull/3#discussion_r73977503
--- Diff: tso-server/src/main/resources/default-omid-server-configuration.yml ---
@@ -144,5 +144,4 @@ metrics: !!org.apache.omid.metrics.NullMetricsProvider [ ]
# zkNamespace: "omid"
# metrics: !!org.apache.omid.metrics.NullMetricsProvider [ ]
-
-
+waitStrategy: HIGH_THROUGHPUT
--- End diff --
I think we should move this option to the line 16 (after the port config) as is one of they key config options now. Add also a comment to help users to use it:
```
# Wait strategy for the Disruptor processors in TSO pipeline. Options:
# 1) HIGH_THROUGHPUT - [Default] Use this in production deployments for maximum performance
# 2) LOW_CPU - Use this option when testing or in deployments where saving CPU cycles is more important than throughput
waitStrategy: HIGH_THROUGHPUT
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by francisco-perez-sorrosal <gi...@git.apache.org>.
Github user francisco-perez-sorrosal commented on a diff in the pull request:
https://github.com/apache/incubator-omid/pull/3#discussion_r73978399
--- Diff: tso-server/src/main/java/org/apache/omid/tso/DisruptorModule.java ---
@@ -18,14 +18,38 @@
package org.apache.omid.tso;
import com.google.inject.AbstractModule;
+import com.google.inject.name.Names;
+import com.lmax.disruptor.BlockingWaitStrategy;
+import com.lmax.disruptor.BusySpinWaitStrategy;
+import com.lmax.disruptor.WaitStrategy;
import javax.inject.Singleton;
public class DisruptorModule extends AbstractModule {
+ private final TSOServerConfig config;
+
+ public DisruptorModule(TSOServerConfig config) {
+ this.config = config;
+ }
+
@Override
protected void configure() {
-
+ switch (config.getWaitStrategyEnum()) {
+ // A low-cpu usage Disruptor configuration for using in local/test environments
+ case LOW_CPU:
+ bind(WaitStrategy.class).annotatedWith(Names.named("PersistenceStrategy")).to(BlockingWaitStrategy.class);
+ bind(WaitStrategy.class).annotatedWith(Names.named("ReplyStrategy")).to(BlockingWaitStrategy.class);
+ bind(WaitStrategy.class).annotatedWith(Names.named("RetryStrategy")).to(BlockingWaitStrategy.class);
+ break;
+ // The default high-cpu usage Disruptor configuration for getting high throughput on production environments
+ case HIGH_THROUGHPUT:
+ default:
+ bind(WaitStrategy.class).annotatedWith(Names.named("PersistenceStrategy")).to(BusySpinWaitStrategy.class);
+ bind(WaitStrategy.class).annotatedWith(Names.named("ReplyStrategy")).to(BusySpinWaitStrategy.class);
+ bind(WaitStrategy.class).annotatedWith(Names.named("RetryStrategy")).to(BusySpinWaitStrategy.class);
--- End diff --
I've realized that this should be a YieldingWaitStrategy according to the current code (see below). So this should be:
```java
bind(WaitStrategy.class).annotatedWith(Names.named("RetryStrategy")).to(YieldingWaitStrategy.class);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by francisco-perez-sorrosal <gi...@git.apache.org>.
Github user francisco-perez-sorrosal commented on a diff in the pull request:
https://github.com/apache/incubator-omid/pull/3#discussion_r74187447
--- Diff: tso-server/src/main/java/org/apache/omid/tso/ReplyProcessorImpl.java ---
@@ -67,7 +69,8 @@
private final Meter timestampMeter;
@Inject
- ReplyProcessorImpl(MetricsRegistry metrics, Panicker panicker, ObjectPool<Batch> batchPool) {
+ ReplyProcessorImpl(@Named("PersistenceStrategy") WaitStrategy strategy,
--- End diff --
Typo in named. Should be the same name that is in the DisruptorModule.java for the reply processor:
```
@Named("ReplyStrategy")
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by francisco-perez-sorrosal <gi...@git.apache.org>.
Github user francisco-perez-sorrosal commented on a diff in the pull request:
https://github.com/apache/incubator-omid/pull/3#discussion_r73977418
--- Diff: tso-server/src/main/java/org/apache/omid/tso/TSOServerConfig.java ---
@@ -37,6 +38,10 @@
private static final String CONFIG_FILE_NAME = "omid-server-configuration.yml";
private static final String DEFAULT_CONFIG_FILE_NAME = "default-omid-server-configuration.yml";
+ public static enum WAIT_STRATEGY {
--- End diff --
Typo: Missing empty line after constants
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by francisco-perez-sorrosal <gi...@git.apache.org>.
Github user francisco-perez-sorrosal commented on a diff in the pull request:
https://github.com/apache/incubator-omid/pull/3#discussion_r73978116
--- Diff: tso-server/src/test/java/org/apache/omid/tso/TestPersistenceProcessor.java ---
@@ -145,7 +145,7 @@ public void testCommitPersistenceWithSingleCommitTableWriter() throws Exception
ObjectPool<Batch> batchPool = spy(new BatchPoolModule(tsoConfig).getBatchPool());
- ReplyProcessor replyProcessor = new ReplyProcessorImpl(metrics, panicker, batchPool);
+ ReplyProcessor replyProcessor = new ReplyProcessorImpl(tsoConfig, metrics, panicker, batchPool);
--- End diff --
According to the changes above regarding to WaitStrategy injections, the *ProcessorImpl clases in all the tests should create the objects of the previous WaitStrategy in each case. This is the matching:
PersistenceProcessorImpl - BlockingWaitStrategy
ReplyProcessorImpl - BlockingWaitStrategy
RetryProcessorImpl - YieldingWaitStrategy
e.g. here should be:
```java
ReplyProcessor replyProcessor = new ReplyProcessorImpl(new BlockingWaitStrategy(), metrics, panicker, batchPool);
```
The same applies to the other *ProcessorImpl instantiations below.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-omid pull request #3: OMID-50: Provide an option to reduce tso-ser...
Posted by francisco-perez-sorrosal <gi...@git.apache.org>.
Github user francisco-perez-sorrosal commented on a diff in the pull request:
https://github.com/apache/incubator-omid/pull/3#discussion_r73977397
--- Diff: tso-server/src/main/java/org/apache/omid/tso/ReplyProcessorImpl.java ---
@@ -67,7 +69,7 @@
private final Meter timestampMeter;
@Inject
- ReplyProcessorImpl(MetricsRegistry metrics, Panicker panicker, ObjectPool<Batch> batchPool) {
+ ReplyProcessorImpl(TSOServerConfig config, MetricsRegistry metrics, Panicker panicker, ObjectPool<Batch> batchPool) {
--- End diff --
Same comment as above. Inject strategy instead of tso config. Should be:
```java
@Inject
ReplyProcessorImpl(@Named("ReplyStrategy") WaitStrategy strategy,
MetricsRegistry metrics,
Panicker panicker,
ObjectPool<Batch> batchPool) {
// ------------------------------------------------------------------------------------------------------------
// Disruptor initialization
// ------------------------------------------------------------------------------------------------------------
ThreadFactoryBuilder threadFactory = new ThreadFactoryBuilder().setNameFormat("reply-%d");
this.disruptorExec = Executors.newSingleThreadExecutor(threadFactory.build());
this.disruptor = new Disruptor<>(EVENT_FACTORY, 1 << 12, disruptorExec, MULTI, strategy);
...
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---