Migrate JMX connector to new APIs #511

takezoe · 2019-03-20T11:15:27Z

No description provided.

findepi

@takezoe thank you for your contribution!

Fix deprecation warning in jmx plugin

I would change the commit message to reflect that you're migrating the JMX connector to the new APIs.

@martint i am sorry, i didn't follow the API changes. Please review.

findepi · 2019-03-20T13:34:15Z

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxMetadata.java

-import io.prestosql.spi.connector.Constraint;
-import io.prestosql.spi.connector.SchemaTableName;
-import io.prestosql.spi.connector.SchemaTablePrefix;
+import io.prestosql.spi.connector.*;


We do not use star imports.
Hopefully importing https://github.com/airlift/codestyle into Intellij should get you the right configuration.

Thanks! Fixed it.

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxMetadata.java

takezoe · 2019-03-20T17:01:05Z

@findepi Thanks for your review! I fixed code and updated the title of this pull request.

Currently some test cases are commented out because it seems that there is no way to give predicate to SplitManager without deprecated version of getSplits() method. Actually, I'm not sure how I can port this part to new APIs.

martint · 2019-03-20T23:12:56Z

presto-jmx/src/test/java/io/prestosql/plugin/jmx/TestJmxSplitManager.java

+//        for (Node node : nodes) {
+//            String nodeIdentifier = node.getNodeIdentifier();
+//
+//            // TODO Is there any way to give predicate to SplitManager without deprecated version of getSplits()?


The new API doesn't yet support pushing down predicates. It's something we're adding as part of #18. We'll add a version of the API that takes a Constraint object like getTableLayout does today to ease migration. We should probably wait to complete and merge this PR until that gets in (which will happen soon), otherwise, we're removing existing features for this connector.

Thanks for the explanation! I understand it isn't a time to migrate connectors yet. I'm waiting for Constraint to be added.

@takezoe, with this, you should be able to complete the migration of this connector: #541.

martint · 2019-04-02T00:50:28Z

@takezoe, #541 is merged, so you should be able to rebase this PR and implement the missing functionality now.

takezoe · 2019-04-02T02:27:38Z

Thanks! I'm going to resume working on this pull request.

ebyhr · 2019-04-13T13:41:07Z

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxTableHandle.java

+    public static class Node
+    {
+        private String identifier;
+        private HostAddress address;


I think these fields should be private final.

ebyhr · 2019-04-13T13:41:21Z

presto-jmx/src/test/java/io/prestosql/plugin/jmx/TestJmxMetadata.java

+        Optional<ConstraintApplicationResult<ConnectorTableHandle>> result = metadata.applyFilter(handle, new Constraint(TupleDomain.all()));
+
+        assertTrue(result.isPresent());
+        assertTrue(((JmxTableHandle) result.get().getHandle()).getNodes().size() == 3);


It would be better to use assertEquals when we check two values. L138 is also the same.

ebyhr · 2019-04-13T13:46:20Z

presto-jmx/src/test/java/io/prestosql/plugin/jmx/TestJmxTableHandle.java

+
+    private static JmxTableHandle.Node createTestingNode(String hostname)
+    {
+        return new JmxTableHandle.Node(hostname, HostAddress.fromString(format("%s:8080", hostname)));


HostAddress.fromParts is simpler than concating hostname and port.

return new JmxTableHandle.Node(hostname, HostAddress.fromParts(hostname, 8080));

ebyhr · 2019-04-13T13:56:02Z

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxSplitManager.java

-                .map(node -> new JmxSplit(ImmutableList.of(node.getHostAndPort())))
-                .collect(toList());
+        List<ConnectorSplit> splits = tableHandle.getNodes().stream()
+                .map(node -> new JmxSplit(ImmutableList.of(node.getAddress()))).collect(toList());


Move .collect(toList()); to next line.

Also, we can use .collect(toImmutableList()); and then change L40 to

return new FixedSplitSource(splits);

Move .collect(toList()); to next line.

Do we have a specific regulation about putting new lines? Actually, I have a similar code in JmxMetadata as follows. I'm wondering if I have to put new lines before .map and .collect.

private List<JmxTableHandle.Node> getAllNodes() { return nodeManager.getAllNodes().stream().map(node -> new JmxTableHandle.Node(node.getNodeIdentifier(), node.getHostAndPort())).collect(toList()); }

When the statement is simple, one line is fine. When you break a line after .stream(), subsequent each operations should be independent line as original code was. (There's no 'specific' regulation as far as I know).

The getAllNodes looks little long for one line and there's only one statement in the method, so putting new lines looks reasonable to me.

ebyhr · 2019-04-13T14:07:11Z

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxRecordSetProvider.java

+            ConnectorSession session,
+            ConnectorSplit split,
+            ConnectorTableHandle table,
+            List<? extends ColumnHandle> columns)


This change looks just fixing style, but the commit title is Fix warning in JmxRecordSetProvider. What is the waring message (line length)?

I can't remember. Let me revert this change.

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxTableHandle.java

ebyhr · 2019-04-13T14:29:05Z

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxMetadata.java

+                })
+                .collect(toList());
+
+        JmxTableHandle newTaleHandle = new JmxTableHandle(tableHandle.getTableName(), tableHandle.getObjectNames(), tableHandle.getColumnHandles(), tableHandle.isLiveData(), nodes);


Typo newTaleHandle → newTableHandle

takezoe · 2019-04-15T02:28:10Z

@martint I finished to migrate the JMX connector node selection pushdown to Metadata.applyFilter. Can you take a look?

martint

Thanks! That's a step in the right direction, but as a I mentioned in the comment above, it'd be better to hold on to the filter in the table handle instead of the list of filtered nodes (which can be large for large installations). and filter them at the point splits are generated.

martint · 2019-04-22T17:24:13Z

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxTableHandle.java


    @JsonCreator
    public JmxTableHandle(
            @JsonProperty("tableName") SchemaTableName tableName,
            @JsonProperty("objectNames") List<String> objectNames,
            @JsonProperty("columnHandles") List<JmxColumnHandle> columnHandles,
            @JsonProperty("liveData") boolean liveData,
-            @JsonProperty("addresses") List<HostAddress> addresses)
+            @JsonProperty("nodes") List<Node> nodes)


Why hold on to the list of nodes in the TableHandle? I'd be better to just hold on to the predicate and filter them out during split generation.

In my understanding, to hold the predicate ( TupleDomain or Map<ColumnHandle, Domain>) in the TableHandle, we need to make all classes held in the ValueSet serializable to JSON. Is this correct?

Oops, I didn't know TupleDomain is always JSON serializable. I fixed to hold TupleDomain in TableHandle instead of the list of node.

electrum · 2019-04-22T22:10:01Z

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxMetadata.java

+    @Override
+    public ConnectorTableProperties getTableProperties(ConnectorSession session, ConnectorTableHandle table)
+    {
+        return new ConnectorTableProperties(


This can use the no-args constructor, which does the same thing

martint · 2019-04-30T21:25:20Z

@takezoe, can you rebase on master and remove the merge commits? They are confusing github and it's not displaying the actual differences with respect to the base commit.

takezoe · 2019-05-01T03:38:26Z

Sure, I rebased on master branch without merge commits.

martint · 2019-05-01T17:51:39Z

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxMetadata.java

-        return ImmutableList.of(new ConnectorTableLayoutResult(layout, constraint.getSummary()));
-    }
+        JmxTableHandle tableHandle = (JmxTableHandle) handle;
+        TupleDomain<ColumnHandle> predicate = constraint.getSummary();


applyFilter can get called multiple times throughout the optimization process. Connectors need to ensure that they "remember" the result of previous applications. Also, if applying the filter did not have any effect, the method needs to return Optional.empty(). Otherwise, the optimizer may loop forever.

See https://github.com/prestosql/presto/blob/master/presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcMetadata.java#L86 for an example.

martint · 2019-05-01T17:53:09Z

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxMetadata.java

-        return new ConnectorTableLayout(handle);
+        JmxTableHandle newTableHandle = new JmxTableHandle(tableHandle.getTableName(), tableHandle.getObjectNames(), tableHandle.getColumnHandles(), tableHandle.isLiveData(), predicate);
+
+        return Optional.of(new ConstraintApplicationResult(newTableHandle, predicate));


If the connector guarantees the results are going to satisfy the constraint, it's not necessary to return the predicate in the ConstraintApplicationResult. It can just indicate TupleDomain.ALL.

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxTableHandle.java

takezoe · 2019-05-02T07:24:40Z

@martint Thanks for your review again. Fixed to return Option.empty if JmxTableHandle has a exact same constraint. Also now JmxTableHandle holds only node column filter and others are passed to ConstraintApplicationResult as remainingFilter to exclude predicates which are processed by the connector.

martint

Sorry for the delay. This looks great. Just a few minor comments, mostly around unrelated changes.

Can you squash all commits? I'll merge it then.

martint · 2019-05-20T18:34:22Z

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxSplitManager.java


-        //TODO is there a better way to get the node column?


unrelated change?

Oh, sorry. Reverted.

martint · 2019-05-20T18:34:34Z

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxSplitManager.java

        Optional<JmxColumnHandle> nodeColumnHandle = tableHandle.getColumnHandles().stream()
                .filter(jmxColumnHandle -> jmxColumnHandle.getColumnName().equals(NODE_COLUMN_NAME))
                .findFirst();
+


Unrelated formatting change.

True. Reverted too.

martint · 2019-05-20T18:37:17Z

presto-jmx/src/test/java/io/prestosql/plugin/jmx/TestJmxTableHandle.java

                .addEquivalentGroup(
-                        new JmxTableHandle(SCHEMA_TABLE_NAME, ImmutableList.of("name"), COLUMNS, false),
-                        new JmxTableHandle(SCHEMA_TABLE_NAME, ImmutableList.of("name"), COLUMNS, false))
+                        new JmxTableHandle(SCHEMA_TABLE_NAME, ImmutableList.of("nameX"), COLUMNS, true, TupleDomain.all()),


Why the name change here?

This is diff magic. I didn't change the name, just added new arguments. "name" patterns are coverd by preceding 2 equivalent groups.

martint · 2019-05-20T18:37:24Z

presto-jmx/src/test/java/io/prestosql/plugin/jmx/TestJmxTableHandle.java

                .addEquivalentGroup(
-                        new JmxTableHandle(SCHEMA_TABLE_NAME, ImmutableList.of("nameX"), COLUMNS, false),


Why the name change here?

Same as above.

Fix warning in JmxRecordSetProvider Removed unused import statements Migrate node column pushdown to applyFilter Fix compilation error Fix JmxTableHandle to have all nodes initially then filter them at applyFilter Add tests for JmxMetadata#applyFilter() Review feedback Add TupleDomain to JmxTableHandle Fix JmxMetadata.getTableProperties to use no-args constructor Fix compilation error Return empty from applyFilter if TableHandle has same constraint Split TupleDomain to node column and others Fix Json property name and error message of nodeFilter Post review feedback

takezoe · 2019-05-21T15:31:51Z

@martint Thanks for your comment! Fixed and squashed.

martint · 2019-05-21T16:14:41Z

Merged. Thanks for your contribution!

cla-bot bot added the cla-signed label Mar 20, 2019

takezoe force-pushed the fix-jmx-warning branch from 7fb01cb to 79046a4 Compare March 20, 2019 13:28

findepi reviewed Mar 20, 2019

View reviewed changes

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxMetadata.java Outdated Show resolved Hide resolved

takezoe force-pushed the fix-jmx-warning branch from 79046a4 to 29efa7d Compare March 20, 2019 16:29

takezoe changed the title ~~Fix deprecation warning in jmx plugin~~ Migrate JMX connector to new APIs Mar 20, 2019

martint reviewed Mar 20, 2019

View reviewed changes

takezoe changed the title ~~Migrate JMX connector to new APIs~~ [WIP] Migrate JMX connector to new APIs Mar 22, 2019

takezoe force-pushed the fix-jmx-warning branch from e2f6cc5 to bc16558 Compare April 2, 2019 13:37

takezoe force-pushed the fix-jmx-warning branch 3 times, most recently from ab2ed22 to c04d402 Compare April 13, 2019 08:04

takezoe changed the title ~~[WIP] Migrate JMX connector to new APIs~~ Migrate JMX connector to new APIs Apr 13, 2019

ebyhr reviewed Apr 13, 2019

View reviewed changes

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxTableHandle.java Show resolved Hide resolved

ebyhr reviewed Apr 13, 2019

View reviewed changes

takezoe force-pushed the fix-jmx-warning branch 2 times, most recently from 3cc894c to 8967050 Compare April 14, 2019 06:30

martint requested changes Apr 22, 2019

View reviewed changes

martint assigned takezoe Apr 22, 2019

electrum reviewed Apr 22, 2019

View reviewed changes

electrum self-requested a review April 22, 2019 22:10

takezoe mentioned this pull request Apr 29, 2019

Remove table layouts from Hive #689

Merged

takezoe force-pushed the fix-jmx-warning branch from e0d60cb to 1e6e77d Compare April 30, 2019 07:03

takezoe force-pushed the fix-jmx-warning branch from 1e6e77d to 2c12d56 Compare May 1, 2019 03:35

martint reviewed May 1, 2019

View reviewed changes

ebyhr reviewed May 2, 2019

View reviewed changes

presto-jmx/src/main/java/io/prestosql/plugin/jmx/JmxTableHandle.java Outdated Show resolved Hide resolved

takezoe force-pushed the fix-jmx-warning branch from f97e27c to 0794e73 Compare May 2, 2019 13:48

martint self-requested a review May 6, 2019 23:07

electrum mentioned this pull request May 16, 2019

Remove table layouts from all connectors #781

Closed

16 tasks

martint reviewed May 20, 2019

View reviewed changes

takezoe force-pushed the fix-jmx-warning branch from 0794e73 to 23e03a0 Compare May 21, 2019 13:37

martint closed this May 21, 2019

takezoe deleted the fix-jmx-warning branch May 22, 2019 07:31

		.addEquivalentGroup(
		new JmxTableHandle(SCHEMA_TABLE_NAME, ImmutableList.of("nameX"), COLUMNS, false),

Migrate JMX connector to new APIs #511

Migrate JMX connector to new APIs #511

Conversation

takezoe commented Mar 20, 2019

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takezoe commented Mar 20, 2019

Choose a reason for hiding this comment

takezoe Mar 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martint commented Apr 2, 2019

takezoe commented Apr 2, 2019 • edited

Choose a reason for hiding this comment

ebyhr Apr 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Apr 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Apr 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Apr 13, 2019 • edited

Choose a reason for hiding this comment

takezoe commented Apr 15, 2019

martint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martint commented Apr 30, 2019

takezoe commented May 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takezoe commented May 2, 2019

martint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takezoe May 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takezoe commented May 21, 2019

martint commented May 21, 2019

takezoe Mar 21, 2019 •

edited

takezoe commented Apr 2, 2019 •

edited

ebyhr Apr 13, 2019 •

edited

ebyhr Apr 13, 2019 •

edited

ebyhr Apr 13, 2019 •

edited

ebyhr Apr 13, 2019 •

edited

takezoe May 21, 2019 •

edited