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.

Sign up for GitHub

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

Jump to bottom

[BEAM-3446] Fixes RedisIO non-prefix read operations #4656

Closed

vvarma wants to merge 1 commit into apache:master from vvarma:i3446

Contributor

vvarma commented Feb 11, 2018

BaseReadFn to abstract general jedis operations.
Moved key fetch given prefix to ReadKeywsWithPattern DoFn.
ReadFn is pure fetch from redis given key.

URL: https://issues.apache.org/jira/browse/BEAM-3446


          Fixes https://issues.apache.org/jira/browse/BEAM-3446.

50d2a11

BaseReadFn to abstract general jedis operations. Separated key fetch using prefix and get by key into serparate DoFn.

jbonofre self-requested a review

February 11, 2018 05:15

lukecwik assigned jbonofre

stale bot commented Jun 7, 2018

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

stale bot added the wontfix label

Contributor Author

vvarma commented Jun 8, 2018

the fix is in place @jbonofre please help review

stale bot removed the wontfix label

Member

kennknowles commented Jun 28, 2018

We have turned on autoformatting of the codebase, which causes small conflicts across the board. You can probably safely rebase and just keep your changes. Like this:

$ git rebase
... see some conflicts
$ git diff
... confirmed that the conflicts are just autoformatting
... so we can just keep our changes are do our own autoformat
$ git checkout --theirs --
$ git add -u
$ git rebase --continue
$ ./gradlew spotlessJavaApply

Please ping me if you run into any difficulty.

iemejia requested changes

View reviewed changes

Member

iemejia left a comment

Hi, sorry for taking so long @vvarma. Can you please rebase this one and take a look at my comments. (@jbonofre sorry for jumping in but was taking a look at broken things because of the recent spotless enable and thought about helping with this one).

sdks/java/io/redis/src/main/java/org/apache/beam/sdk/io/redis/RedisIO.java

                     return input
                         .apply(Create.of(keyPattern()))
+                        .apply(ParDo.of(new ReadKeywsWithPattern(connectionConfiguration())))

Member

iemejia Jun 29, 2018

s/ReadKeywsWithPattern/ReadKeysWithPattern

sdks/java/io/redis/src/main/java/org/apache/beam/sdk/io/redis/RedisIO.java

-                  private final RedisConnectionConfiguration connectionConfiguration;
+                private abstract static class BaseReadFn<T> extends DoFn<String, T> {
+                  protected final RedisConnectionConfiguration connectionConfiguration;

Member

iemejia Jun 29, 2018

can be package private

sdks/java/io/redis/src/main/java/org/apache/beam/sdk/io/redis/RedisIO.java


		private transient Jedis jedis;
		protected transient Jedis jedis;

Member

iemejia Jun 29, 2018

can be package private

sdks/java/io/redis/src/main/java/org/apache/beam/sdk/io/redis/RedisIO.java


		public ReadFn(RedisConnectionConfiguration connectionConfiguration) {
		public BaseReadFn(RedisConnectionConfiguration connectionConfiguration) {

Member

iemejia Jun 29, 2018

remove public, in general it is a common Beam practice to restrict access as much as possible. You can use IntelliJ's analyze code to do this.

sdks/java/io/redis/src/main/java/org/apache/beam/sdk/io/redis/RedisIO.java

                     while (!finished) {
                       ScanResult<String> scanResult = jedis.scan(cursor, scanParams);
                       List<String> keys = scanResult.getResult();
-                      Pipeline pipeline = jedis.pipelined();

Member

iemejia Jun 29, 2018

Question: What is the reason to remove pipelining in general, seems like if the approach of this PR is more composable, it would perform worse, won't it.

Contributor Author

vvarma Jun 30, 2018

The number of keys that need to be looked up in a given window or batch can vary. Ideally, we should have a configurable batch size and use MGET https://redis.io/commands/mget if wanted to optimize further.
Pipelining an entire window or batch can cause memory spikes in Redis depending on the number of keys being looked up, for the time being, to simplify things I removed pipeline.

vvarma mentioned this pull request

[BEAM-3446] Fixes RedisIO non-prefix read operations #5841

Merged

Contributor Author

vvarma commented Jun 30, 2018

Closing this pr, opened a new one with fixes and rebased. #5841

vvarma closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment