Skip to content
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

Issue 32: Update master branch with contents of dev branch #33

Merged
merged 29 commits into from
Jul 17, 2020

Conversation

RaulGracia
Copy link
Contributor

Change log description
This PR contains the changes from dev branch to be merged on master to reproduce the experiments to be published:

  • Added new driver for Pravega.
  • Added transactional writer to Kafka.
  • Improvement on metrics reporting.

Still, we need to remove all the non-necessary changes to make this fork as close as possible from the official repo. We also need to remove any mention to personal repos and create the necessary images in Pravega dockerhub.

Claudio Fahey and others added 18 commits November 14, 2019 20:32
)

Import of current code to development branch with all work done so far.

Signed-off-by: Claudio Fahey <[email protected]>
Added transaction support for Pravega driver in OpenMessaging Benchmark.

Signed-off-by: wenqimou <[email protected]>
Added transaction support for Kafka.

Signed-off-by: wenqimou <[email protected]>
Remove P3 Test Driver framework from OpenMessaging Benchmark fork.
…nnection pooling for writers and time windows for readers.

Signed-off-by: Raúl Gracia <[email protected]>
Issue 11: Enable stream auto-scaling
Refractor README
Collocate Segment Store and Bookies and set proper config changes.

Signed-off-by: wenqimou <[email protected]>
…default_b_dev

Fixing issue 18: PravegaConfig.enableTransaction has the default value of true
Increase sleep time to avoid a race condition when a test completes
* Add option to create scope
* Add keycloak dependency
)

Avoid allocating a new byte buffer for each write for efficiency reasons.

Signed-off-by: Raúl Gracia <[email protected]>
RaulGracia and others added 5 commits June 25, 2020 16:04
Add configuration to enable Tiering in Pulsar.

Signed-off-by: Raúl Gracia <[email protected]>
Co-authored-by: Malygina <[email protected]>
@RaulGracia RaulGracia force-pushed the issue-32-cleanup-dev branch from 9768588 to 2b35ce7 Compare June 25, 2020 14:25
@RaulGracia RaulGracia marked this pull request as ready for review June 25, 2020 14:27
.gitignore Outdated Show resolved Hide resolved
private final ExecutorService executor = Executors.newCachedThreadPool(new DefaultThreadFactory("local-worker"));
// private final ExecutorService executor = Executors.newCachedThreadPool(new DefaultThreadFactory("local-worker"));
// private final ExecutorService executor = Executors.newFixedThreadPool(16);
private final ExecutorService executor = new ForkJoinPool();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should explain why we changed from newCachedThreadPool to ForkJoinPool. I think I copied this from Pravega Benchmark.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but what was the reason this change was introduced? The fact that Pravega Benchmark did this does not seem a sufficiently strong reason, right? Is this providing any advantage over the existing thread pool?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I don't remember exactly why I started using ForkJoinPool. I was likely investigating performance differences between OMB and Pravega Benchmark and I found this to work well. I agree that we should better justify the change or revert it. Are you able to run a benchmark with the original executor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will need to do that to see if there is any difference.

bin/benchmark-worker Outdated Show resolved Hide resolved
driver-pravega/doc/build_pravega.md Outdated Show resolved Hide resolved
driver-pulsar/deploy/deploy.yaml Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
workloads/tiny.yaml Outdated Show resolved Hide resolved
@jingerbread
Copy link
Contributor

jingerbread commented Jul 8, 2020

[updated] claudiofahey/pravega-tester:0.7.0 from 0.5.1 version
[changed] pravegaSrcRemote: yes pravegaSrc: https://github.com/pravega/pravega/releases/download...

@RaulGracia
Copy link
Contributor Author

@jingerbread @claudiofahey I don't think that is a good idea to rely on images from personal repos. If we need an image with whatever is in pravega-tester, then we could consider creating one in the official Pravega repo.
Also @jingerbread, the change on pravegaSrcRemote is ok but will take effect once we release Pravega 0.8.

@claudiofahey
Copy link

@jingerbread @claudiofahey I don't think that is a good idea to rely on images from personal repos. If we need an image with whatever is in pravega-tester, then we could consider creating one in the official Pravega repo.

It will take some time to get approvals to make pravega-tester official. Let's remove it from the Ansible script for now. It is not too important. Pravega-tester just performs a few writes and reads from Pravega as a sanity check.

@jingerbread
Copy link
Contributor

jingerbread commented Jul 15, 2020

@RaulGracia, currently in the PR pravegaVersion: "0.7.1" which is released already and using pravegaSrcRemote https://github.com/pravega/pravega/releases/download/v0.7.1/pravega-0.7.1.tgz will work

@RaulGracia
Copy link
Contributor Author

@jingerbread why have you closed this PR that is not yours and is still under review?

@RaulGracia RaulGracia reopened this Jul 15, 2020
@jingerbread
Copy link
Contributor

@jingerbread why have you closed this PR that is not yours and is still under review?

Sorry, I did it by mistake

@RaulGracia
Copy link
Contributor Author

@claudiofahey @jingerbread we can leave at the moment the Pravega tester image as it is, given that it is useful to know if the deployment works. We can address updating that image on a separate issue: #35.

@jingerbread
Copy link
Contributor

jingerbread commented Jul 15, 2020

@RaulGracia, should
export TF_STATE=./ to

be added before
ansible-playbook --user ec2-user --inventory `which terraform-inventory` deploy.yaml

In README.md for https://github.com/pravega/openmessaging-benchmark/tree/issue-32-cleanup-dev/driver-pravega#running-the-ansible-playbook

Now it's in separate Troubleshooting page https://github.com/pravega/openmessaging-benchmark/blob/issue-32-cleanup-dev/driver-pravega/doc/troubleshooting.md

But for proposed in README.md terraform and terraform-inventory versions
ansible-playbook --user ec2-user --inventory `which terraform-inventory` deploy.yaml
will do nothing, because can't get hosts from inventory

[root@W10J6T4733 deploy]# ansible-playbook  --user ec2-user --inventory `which terraform-inventory` deploy.yaml
 [WARNING]:  * Failed to parse /home/aws/tools/terraform-inventory with script plugin: Inventory script (/home/aws/tools/terraform-inventory) had an execution error: Error reading tfstate file: 0.12 format error: <nil>; pre-0.12 format error: <nil> (nil error means no content/modules found in the respective format)

@jingerbread
Copy link
Contributor

Should io.pravega:pravega-client version in driver-pravega/pom.xml changed from 0.6.2 version to 0.7.1?

        <dependency>
            <groupId>io.pravega</groupId>
            <artifactId>pravega-client</artifactId>
            <version>0.6.2</version>
        </dependency>

To

<!-- https://mvnrepository.com/artifact/io.pravega/pravega-client -->
<dependency>
    <groupId>io.pravega</groupId>
    <artifactId>pravega-client</artifactId>
    <version>0.7.1</version>
</dependency>

@jingerbread
Copy link
Contributor

jingerbread commented Jul 15, 2020

For Apache Maven 3.6.3

mvn clean install -DskipTests=true

failed on driver-pulsar due to

[ERROR] Failed to execute goal com.mycila:license-maven-plugin:3.0:check (default) on project driver-pulsar: 
Some files do not have the expected license header -> [Help 1]

Need to

 mvn clean install -DskipTests=true -Dlicense.skip=true

@jingerbread
Copy link
Contributor

Seems, when Pravega 0.8 will be release, need to change controller.zk.url to controller.zk.connect.uri in templates/controller.config.properties. Change also zookeeper version in vars.yaml to 3.6.1.

May be separate task to update this PR for 0.8 can be created?

[root@W10J6T4733 deploy]# grep -r "controller.zk.url" *
templates/controller.config.properties:controller.zk.url={{ zookeeperServers }}

pravega/pravega@b4cd667
Issue 4900: Fix configuration related issues in templates and docs

...
- "value": "SERVER_OPTS=\"-Dcontroller.zk.url=master.mesos:2181\" ...
+ "value": "SERVER_OPTS=\"-Dcontroller.zk.connect.uri=master.mesos:2181\ ...

Otherwise, now with latest Pravega commit b4cd667 from 15 Jul - Pravega deployment wont't work.
Controller will try to connect to zk on localhost:

2020-07-15 18:13:33,972 319  [main] INFO  io.pravega.controller.util.Config - controller.zk.url = 10.0.0.152:2181,10.0.0.86:2181,10.0.0.105:2181
...
2020-07-15 18:13:34,324 671  [ControllerServiceMain-SendThread(localhost:2181)] INFO  org.apache.zookeeper.ClientCnxn - Socket error occurred: localhost/0:0:0:0:0:0:0:1:2181: Connection refused
2020-07-15 18:13:34,327 674  [ControllerServiceMain] INFO  i.p.c.server.ControllerServiceMain - Awaiting ZK client connection to ZK server
2020-07-15 18:13:35,426 1773 [ControllerServiceMain-SendThread(localhost:2181)] INFO  org.apache.zookeeper.ClientCnxn - Opening socket connection to server localhost/127.0.0.1:2181. Will not attempt to authenticate using SASL (unknown error)
2020-07-15 18:13:35,427 1774 [ControllerServiceMain-SendThread(localhost:2181)] INFO  org.apache.zookeeper.ClientCnxn - Socket error occurred: localhost/127.0.0.1:2181: Connection refused

However, current setting should work with Pravega 0.7.1, which is currently referenced in vars.yaml

@jingerbread
Copy link
Contributor

With mentioned changes - deployment was successful for latest Pravega commit b4cd667 from 15 Jul

PLAY [Run Pravega Tester] *****************************************************************************************************************************************************************************************************************************************************************************************************

....
PLAY RECAP ********************************************************************************************************************************************************************************************************************************************************************************************************************34.214.51.197              : ok=45   changed=14   unreachable=0    failed=0    skipped=18   rescued=0    ignored=2
34.216.225.48              : ok=34   changed=11   unreachable=0    failed=0    skipped=18   rescued=0    ignored=2
34.217.32.131              : ok=41   changed=15   unreachable=0    failed=0    skipped=16   rescued=0    ignored=2
34.219.134.75              : ok=35   changed=11   unreachable=0    failed=0    skipped=16   rescued=0    ignored=2
34.219.162.179             : ok=41   changed=15   unreachable=0    failed=0    skipped=16   rescued=0    ignored=2
34.219.171.194             : ok=52   changed=23   unreachable=0    failed=0    skipped=15   rescued=0    ignored=1
34.220.119.134             : ok=50   changed=21   unreachable=0    failed=0    skipped=17   rescued=0    ignored=1
52.38.10.6                 : ok=34   changed=11   unreachable=0    failed=0    skipped=18   rescued=0    ignored=2
52.39.178.9                : ok=50   changed=21   unreachable=0    failed=0    skipped=17   rescued=0    ignored=1
54.201.55.191              : ok=36   changed=13   unreachable=0    failed=0    skipped=16   rescued=0    ignored=2
localhost                  : ok=4    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

@RaulGracia
Copy link
Contributor Author

@jingerbread let me answer your comments:

  • Agree, export TF_STATE=./ seems a must rather than a corner case to be added in the troubleshooting guide.
  • Regarding io.pravega:pravega-client version, it should actually be 0.8.0 when it is released, but we can use 0.7.1 at the moment to merge this PR. We will update the version later on.
  • Regarding -Dlicense.skip=true, if it works also with earlier version of Maven, I'm fine with it too.
  • Right, we need to change controller.zk.connect.uri for 0.8.0. But this should be done in a separate PR when we upgrade the Pravega artifacts to 0.8.0. Can you file an issue to track that?
  • Regarding the Zookeeper version, we can leave the one we have, is not important for performance and the current version works well.

@jingerbread as you have these changes in place already, can you commit them to this PR please?

@jingerbread
Copy link
Contributor

jingerbread commented Jul 16, 2020

@RaulGracia

Sure, will commit and create separate ticket for 0.8 changes by the MSK end of the day.

@jingerbread
Copy link
Contributor

jingerbread commented Jul 16, 2020

-Dlicense.skip=true is the com.mycila plugin flag to skip checks, see "Request for a skip check goal. #28" mathieucarbou/license-maven-plugin#28 closed in 2013, so think it will work.
In omb com.mycila.license-maven-plugin:3.0 is used for checking license headers:

<plugin>
	<groupId>com.mycila</groupId>
	<artifactId>license-maven-plugin</artifactId>
	<version>3.0</version>
...

Besides, may be we can fix warnings by adding license headers?

[WARNING] Missing header in: C:/DELL_EMC/Development/OMB/pravega-omb/driver-pulsar/deploy/templates/chrony.conf
[WARNING] Missing header in: C:/DELL_EMC/Development/OMB/pravega-omb/driver-kafka/deploy/templates/chrony.conf
[WARNING] Missing header in: C:/DELL_EMC/Development/OMB/pravega-omb/driver-pravega/deploy/templates/bkenv.sh
...

@jingerbread
Copy link
Contributor

jingerbread commented Jul 16, 2020

Created Update driver-pravega after 0.8.0 Pravega release #36 issue
#36

[changed] driver-pravega/README.md: [added] export TF_STATE=./ before ansible deploy step
[changed] driver-pravega/doc/build_pravega.md [added] pravegaSrcRemote and pravegaSrc changing for local artifacts comment [added] comment about -Dlicense.skip=true
@RaulGracia
Copy link
Contributor Author

@jingerbread @claudiofahey I think we can merge this one as the remaining points can be addressed as separate PRs against master branch. What do you think?

@jingerbread
Copy link
Contributor

jingerbread commented Jul 17, 2020

Seem working for 0.7.1, I thing can be merged. Didn't find how to approve this PR.
Added comment about adding license headers to Update driver-pravega after 0.8.0 Pravega release #36 issue

PLAY [Run Pravega Tester] *****************************************************************************************************************************************************************************************************************************************************************************************************

TASK [Gathering Facts] ********************************************************************************************************************************************************************************************************************************************************************************************************ok: [34.212.167.25]

...
PLAY RECAP ********************************************************************************************************************************************************************************************************************************************************************************************************************18.237.21.18               : ok=39   changed=20   unreachable=0    failed=0    skipped=17   rescued=0    ignored=2
34.212.167.25              : ok=48   changed=27   unreachable=0    failed=0    skipped=13   rescued=0    ignored=2
34.217.56.196              : ok=56   changed=32   unreachable=0    failed=0    skipped=12   rescued=0    ignored=3
34.222.245.205             : ok=40   changed=22   unreachable=0    failed=0    skipped=13   rescued=0    ignored=2
52.11.157.73               : ok=39   changed=20   unreachable=0    failed=0    skipped=13   rescued=0    ignored=2
52.12.159.16               : ok=38   changed=20   unreachable=0    failed=0    skipped=15   rescued=0    ignored=2
52.12.248.39               : ok=45   changed=25   unreachable=0    failed=0    skipped=13   rescued=0    ignored=2
54.149.212.20              : ok=45   changed=25   unreachable=0    failed=0    skipped=13   rescued=0    ignored=2
54.213.211.242             : ok=54   changed=30   unreachable=0    failed=0    skipped=14   rescued=0    ignored=3
54.213.57.95               : ok=54   changed=30   unreachable=0    failed=0    skipped=14   rescued=0    ignored=3
localhost                  : ok=4    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0


real    22m2.937s

@RaulGracia RaulGracia requested review from claudiofahey and removed request for Tristan1900 and leapsky July 17, 2020 14:49
@RaulGracia RaulGracia merged commit 4a92e6d into master Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants