Refactor to use setOnce by pgomulka · Pull Request #7 · pgomulka/elasticsearch

pgomulka · 2019-01-23T14:41:51Z

Have you signed the contributor license agreement?
Have you followed the contributor guidelines?
If submitting code, have you built your formula locally prior to submission with gradle check?
If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
If submitting code, have you checked that your submission is for an OS that we support?
If you are submitting this code for a class then read our policy for that.

pgomulka · 2019-01-23T16:05:23Z

server/src/main/java/org/elasticsearch/common/logging/NodeAndClusterIdStateListener.java

+     * Subscribes for the first cluster state update where nodeId and clusterId is set.
+     */
+    public static void subscribeTo(ClusterStateObserver observer) {
+        observer.waitForNextChange(new NodeAndClusterIdStateListener(), NodeAndClusterIdStateListener::nodeIdAndClusterIdSet);


should this have a timeout?

I think it's ok not to specify a timeout here.

rjernst · 2019-01-23T16:06:51Z

server/src/main/java/org/elasticsearch/common/logging/NodeAndClusterIdStateListener.java

+        observer.waitForNextChange(new NodeAndClusterIdStateListener(), NodeAndClusterIdStateListener::nodeIdAndClusterIdSet);
+    }
+
+    private static boolean nodeIdAndClusterIdSet(ClusterState clusterState) {


"set" here threw me off at first (sounds like a setter). Maybe have this be more a boolean name, like "isNodeAndClusterIdReady"?

maybe isNodeAndClusterIdPresent ?

rjernst · 2019-01-23T16:07:20Z

server/src/main/java/org/elasticsearch/common/logging/NodeAndClusterIdConverter.java

     */
-    public static boolean setOnce(String nodeId, String clusterUUID) {
-        return nodeAndClusterId.compareAndSet(null, formatIds(clusterUUID, nodeId));
+    public static void setOnce(String nodeId, String clusterUUID) {


I still dont' like this being "setOnce", I think it should be parallel to the node name listener?

as this is almost the same as NodeNameConverter I think you are right this method should be name in similar manner. Will rename to setNodeIdAndClusterId

rjernst · 2019-01-23T16:10:45Z

server/src/main/java/org/elasticsearch/node/Node.java

                    throw new ElasticsearchTimeoutException("Interrupted while waiting for initial discovery state");
                }
+
+                NodeAndClusterIdStateListener.subscribeTo(observer);


The name subscribeTo looks odd here, since this is a one time use. Perhaps the name should reflect that, like "getAndSetClusterIds"?

Also, note that this is inside a block which will not run if the join has already happened. I think this needs to be moved outside this block.

danielmitterdorfer

Makes sense in general. I left a few minor comments

danielmitterdorfer · 2019-01-23T16:10:34Z

server/src/main/java/org/elasticsearch/common/logging/NodeAndClusterIdStateListener.java

-            logger.debug("Received first cluster state update. Setting nodeId=[{}] and clusterUuid=[{}]", nodeId, clusterUUID);
-        }
+        NodeAndClusterIdConverter.setOnce(nodeId, clusterUUID);
+        logger.debug("Received first cluster state update. Setting nodeId=[{}] and clusterUuid=[{}]", nodeId, clusterUUID);


I'd prefer to move this up before we set it because this statement is never executed if there is a second attempt and we're failing. If it is one line up it would provide at least some ability to see what's going on.

good pint. will remove the first word too

danielmitterdorfer · 2019-01-23T16:11:52Z

server/src/main/java/org/elasticsearch/common/logging/NodeAndClusterIdConverter.java

    /**
-     * Updates only once the clusterID and nodeId
+     * Updates only once the clusterID and nodeId.
+     * Note: Should only be called once. Subsequent executions will throw {@link org.apache.lucene.util.SetOnce.AlreadySetException}.


Nit: Scratch "Note: Should only be called once."?

right, the previous line should make it clear that it should only be called once

danielmitterdorfer · 2019-01-23T16:14:14Z

server/src/main/java/org/elasticsearch/common/logging/NodeAndClusterIdStateListener.java

+     * Subscribes for the first cluster state update where nodeId and clusterId is set.
+     */
+    public static void subscribeTo(ClusterStateObserver observer) {
+        observer.waitForNextChange(new NodeAndClusterIdStateListener(), NodeAndClusterIdStateListener::nodeIdAndClusterIdSet);


I think it's ok not to specify a timeout here.

…astic#69765) Previously we did not resolve the attributes recursively which meant that if a field or expression was re-aliased multiple times (through multiple levels of subqueries), the aliases were only resolved one level down. This led to failed query translation because `ReferenceAttribute`s were pointing to non-existing attributes during query translation. For example the query ```sql SELECT i AS j FROM ( SELECT int AS i FROM test) ORDER BY j ``` failed during translation because the `OrderBy` resolved the `j` ReferenceAttribute to another `i` ReferenceAttribute that was later removed by an Optimization: ``` OrderBy[[Order[j{r}#4,ASC,LAST]]] ! OrderBy[[Order[i{r}#2,ASC,LAST]]] \_Project[[j]] = \_Project[[j]] \_Project[[i]] ! \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] ! ``` By resolving the `Attributes` recursively both `j{r}` and `i{r}` will resolve to `test.int{f}` above: ``` OrderBy[[Order[test.int{f}#22,ASC,LAST]]] = OrderBy[[Order[test.int{f}#22,ASC,LAST]]] \_Project[[j]] = \_Project[[j]] \_Project[[i]] ! \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] ! ``` The scope of recursive resolution depends on how the `AttributeMap` is constructed and populated. Fixes elastic#67237

…astic#69765) (elastic#70322) Previously we did not resolve the attributes recursively which meant that if a field or expression was re-aliased multiple times (through multiple levels of subqueries), the aliases were only resolved one level down. This led to failed query translation because `ReferenceAttribute`s were pointing to non-existing attributes during query translation. For example the query ```sql SELECT i AS j FROM ( SELECT int AS i FROM test) ORDER BY j ``` failed during translation because the `OrderBy` resolved the `j` ReferenceAttribute to another `i` ReferenceAttribute that was later removed by an Optimization: ``` OrderBy[[Order[j{r}#4,ASC,LAST]]] ! OrderBy[[Order[i{r}#2,ASC,LAST]]] \_Project[[j]] = \_Project[[j]] \_Project[[i]] ! \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] ! ``` By resolving the `Attributes` recursively both `j{r}` and `i{r}` will resolve to `test.int{f}` above: ``` OrderBy[[Order[test.int{f}#22,ASC,LAST]]] = OrderBy[[Order[test.int{f}#22,ASC,LAST]]] \_Project[[j]] = \_Project[[j]] \_Project[[i]] ! \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] ! ``` The scope of recursive resolution depends on how the `AttributeMap` is constructed and populated. Fixes elastic#67237

…astic#69765) (elastic#70325) Previously we did not resolve the attributes recursively which meant that if a field or expression was re-aliased multiple times (through multiple levels of subqueries), the aliases were only resolved one level down. This led to failed query translation because `ReferenceAttribute`s were pointing to non-existing attributes during query translation. For example the query ```sql SELECT i AS j FROM ( SELECT int AS i FROM test) ORDER BY j ``` failed during translation because the `OrderBy` resolved the `j` ReferenceAttribute to another `i` ReferenceAttribute that was later removed by an Optimization: ``` OrderBy[[Order[j{r}#4,ASC,LAST]]] ! OrderBy[[Order[i{r}#2,ASC,LAST]]] \_Project[[j]] = \_Project[[j]] \_Project[[i]] ! \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] ! ``` By resolving the `Attributes` recursively both `j{r}` and `i{r}` will resolve to `test.int{f}` above: ``` OrderBy[[Order[test.int{f}#22,ASC,LAST]]] = OrderBy[[Order[test.int{f}#22,ASC,LAST]]] \_Project[[j]] = \_Project[[j]] \_Project[[i]] ! \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] ! ``` The scope of recursive resolution depends on how the `AttributeMap` is constructed and populated. Fixes elastic#67237

…pare_more Spacetime transactions prepare more

pgomulka changed the title ~~Fix/observer logging~~ Refactor to use setOnce Jan 23, 2019

refactor cluster state listeners to use setOnce

f01a4ff

pgomulka force-pushed the fix/observer-logging branch from 080a501 to f01a4ff Compare January 23, 2019 14:45

pgomulka commented Jan 23, 2019

View reviewed changes

rjernst reviewed Jan 23, 2019

View reviewed changes

danielmitterdorfer reviewed Jan 23, 2019

View reviewed changes

pgomulka added 2 commits January 23, 2019 18:12

methods rename and cleanup

88d1368

javadoc typo

72bd776

pgomulka merged commit ee3322c into feature/logging-structured Jan 23, 2019

pgomulka pushed a commit that referenced this pull request Nov 19, 2021

Merge pull request #7 from henningandersen/spacetime_transactions_pre…

84a09e1

…pare_more Spacetime transactions prepare more

Conversation

pgomulka commented Jan 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants