-
Notifications
You must be signed in to change notification settings - Fork 195
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
Deprequeue optimizations #1703
base: main
Are you sure you want to change the base?
Deprequeue optimizations #1703
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
4e4f739
to
7df0ff0
Compare
.bazelci/run_abseil_test.sh
Outdated
../../../bazel test --jobs=25 $1 --test_output=errors --incompatible_enable_cc_toolchain_resolution --verbose_failures --verbose_explanations --test_tag_filters=-benchmark --remote_executor=grpc://localhost:8980 @com_google_absl//... -- -@com_google_absl//absl/time/... No newline at end of file | ||
../../../bazel test --jobs=25 $1 --test_output=errors --incompatible_enable_cc_toolchain_resolution --verbose_failures --verbose_explanations --test_tag_filters=-benchmark --remote_executor=grpc://localhost:8980 @com_google_absl//... -- -@com_google_absl//absl/time/... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary trailing newline add/remove
@@ -49,6 +49,7 @@ public enum BACKPLANE_TYPE { | |||
@Getter(AccessLevel.NONE) | |||
private boolean runFailsafeOperation = true; // deprecated | |||
|
|||
// DBG_AVA remove maxQueueDepth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug
@@ -46,8 +46,9 @@ | |||
* the same underlying redis queues. | |||
*/ | |||
public class BalancedRedisQueue { | |||
private static final Duration START_TIMEOUT = Duration.ofSeconds(1); | |||
public static final int UNLIMITED_QUEUE_DEPTH = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consolidate this value from the config - they shouldn't have different representations
// We assume the number of elements in the queue is evenly distributed across the Redis nodes. | ||
// As such, we're just going to check against against the maximum number of elements an | ||
// individual Redis node should have rather than across all the Redis nodes. | ||
// DBG_AVA handle the case where a maxNodeQueueDepth is 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug
} | ||
return hexString.toString(); | ||
} catch (NoSuchAlgorithmException e) { | ||
throw new RuntimeException("SHA-1 algorithm not found", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit funky - is there nothing in jedis to help with doing this hashing in a way that doesn't bind us to the whims of a redis impl digest function?
|
||
# DBG_AVA | ||
# java_binary( | ||
# name = "fill-prequeue", | ||
# srcs = ["FillPrequeue.java"], | ||
# main_class = "build.buildfarm.tools.FillPrequeue", | ||
# visibility = ["//visibility:public"], | ||
# deps = [ | ||
# "//src/main/java/build/buildfarm/common/config", | ||
# "//src/main/java/build/buildfarm/common/resources:resource_java_proto", | ||
# "//src/main/java/build/buildfarm/instance/shard", | ||
# "//src/main/protobuf:build_buildfarm_v1test_buildfarm_java_proto", | ||
# "@com_google_googleapis//google/longrunning:operations_proto", | ||
# "@maven//:com_google_guava_guava", | ||
# "@maven//:com_google_protobuf_protobuf_java", | ||
# "@maven//:redis_clients_jedis", | ||
# ], | ||
# ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug
@@ -358,11 +358,14 @@ message ExecuteEntry { | |||
string stderr_stream_name = 8; | |||
|
|||
google.protobuf.Timestamp queued_timestamp = 9; | |||
|
|||
optional build.bazel.remote.execution.v2.Digest queued_operation_digest = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove optional - support is deprecated for it in proto3
} | ||
|
||
message QueueEntry { | ||
ExecuteEntry execute_entry = 1; | ||
|
||
// DBG_AVA reserved 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug
// when(subQueue.size()).thenReturn(123L); | ||
|
||
// ARRANGE | ||
BalancedRedisQueue queue = | ||
new BalancedRedisQueue("test", ImmutableList.of("test"), 123, this::subQueueDecorate); | ||
// BalancedRedisQueue queue = | ||
// new BalancedRedisQueue("test", ImmutableList.of("test"), 123, this::subQueueDecorate); | ||
|
||
// ACT | ||
boolean canQueue = queue.canQueue(redis); | ||
// DBG_AVA replace with attempt to queue boolean canQueue = queue.canQueue(redis); | ||
|
||
// ASSERT | ||
verify(subQueue, times(1)).size(); | ||
assertThat(canQueue).isFalse(); | ||
// DBG_AVA verify(subQueue, times(1)).size(); | ||
// DBG_AVA assertThat(canQueue).isFalse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug and cleanup
/** | ||
* @class RedisLuaScriptTest | ||
* @brief exercises the RedisLuaScript class | ||
* @details DBG_AVA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug
7df0ff0
to
6f13995
Compare
6f13995
to
4a99ca4
Compare
NO-FLIGHT