scwhittle commented on code in PR #33689:
URL: https://github.com/apache/beam/pull/33689#discussion_r1925120846
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/StateNamespaces.java:
##########
@@ -156,14 +178,24 @@ public String toString() {
public static class WindowAndTriggerNamespace<W extends BoundedWindow>
implements StateNamespace {
private static final int TRIGGER_RADIX = 36;
- private Coder<W> windowCoder;
- private W window;
- private int triggerIndex;
+ private final Coder<W> windowCoder;
+ private final W window;
+ private final int triggerIndex;
+ private SoftReference<String> stringKeyRef;
Review Comment:
as discussed, maybe we shouldn't use softrefs without some more
investigation.
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/StateNamespaces.java:
##########
@@ -104,17 +118,25 @@ public W getWindow() {
@Override
public String stringKey() {
- try {
- // equivalent to String.format("/%s/", ...)
- return "/" + CoderUtils.encodeToBase64(windowCoder, window) + "/";
- } catch (CoderException e) {
- throw new RuntimeException("Unable to generate string key from window
" + window, e);
- }
+ return getStringKey();
}
@Override
public void appendTo(Appendable sb) throws IOException {
- sb.append('/').append(CoderUtils.encodeToBase64(windowCoder,
window)).append('/');
+ sb.append(getStringKey());
+ }
+
+ private String getStringKey() {
Review Comment:
just inline in stringKey?
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/StateNamespaces.java:
##########
@@ -104,17 +118,25 @@ public W getWindow() {
@Override
public String stringKey() {
- try {
- // equivalent to String.format("/%s/", ...)
- return "/" + CoderUtils.encodeToBase64(windowCoder, window) + "/";
- } catch (CoderException e) {
- throw new RuntimeException("Unable to generate string key from window
" + window, e);
- }
+ return getStringKey();
}
@Override
public void appendTo(Appendable sb) throws IOException {
- sb.append('/').append(CoderUtils.encodeToBase64(windowCoder,
window)).append('/');
+ sb.append(getStringKey());
+ }
+
+ private String getStringKey() {
+ String stringKey = stringKeyRef.get();
Review Comment:
do you need some annotations to make this not show up as racy? we could have
multiple threads calling this at the same time.
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/StateNamespaces.java:
##########
@@ -219,6 +238,27 @@ public boolean equals(@Nullable Object obj) {
&& Objects.equals(this.windowStructuralValue(),
that.windowStructuralValue());
}
+ private String getStringKey() {
Review Comment:
nit: just move this impl into stringKey()?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
