You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/08/11 07:20:49 UTC
[GitHub] [pulsar] eolivelli opened a new pull request #11636: Java Client: remove usage of reflection for loading Pulsar Implementation classes
eolivelli opened a new pull request #11636:
URL: https://github.com/apache/pulsar/pull/11636
- load the DefaultImplementation only once with reflection
- use standard Java calls to access the Pulsar Implementation classes
- reduce the overall overhead of using Reflection
- clarify the relationsship between the classes
- add explicit linking between the impl and api classes, this way it is easier to read the code and perform refactorings
- allow to handle correctly the Exception thrown by implementation classes
### Motivation
Currently the link between the Pulsar Client API and the Implementation is done at runtime, using Java reflection, in the DefaultImplementation class.
This comes with a few problems:
- it is not clear the Classloader who is loading the classes of the Implementation, DefaultImplementation relied on "RelectionUtils", with surprising effects we saw during the past months, expecially in Pulsar Functions/Pulsar II
- the old DefaultImplementation class used to catch all the checked exception and hide them
- the old reflection approach used "strings" to refer to implementation classes, making it harder to perform refactorings, understand the code flow (by reading), using the help of the IDE
- using reflection is generally "slower", with this fix we are going to save CPU cycles, and also the JVM will be able to understand better what's happening and (probably) perform more runtime optimizations (at least we are going to save a few method calls and stack...)
### Modifications
- Introduce a new Java interface `PulsarClientImplementationBinding` that defines the connections from the "public API" and the "implementation", this interface contains all the methods of the old "DefaultImplementation"
- Move the Implementation to the pulsar-client module, in the concrete class `PulsarClientImplementationBindingImpl`
- `PulsarClientImplementationBindingImpl` can access directly, without reflection all the implementation classes, this is faster and also it allows us to have direct linking (for human reading, for IDE help....)
- Load the `PulsarClientImplementationBindingImpl` class only once
- Rewrite all the references to the old `DefaultImplementation.staticMethod(xxx)` to `DefaultImplementation.getDefaultImplementation().method(xxx)`
- Fix some places in which hidden IOException may have bubbled up breaking things without notice
### Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
### Documentation
No need for docs
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #11636: Java Client: remove usage of reflection while using Pulsar Implementation classes
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #11636:
URL: https://github.com/apache/pulsar/pull/11636#discussion_r687966915
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java
##########
@@ -0,0 +1,348 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.impl;
+
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.charset.Charset;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.util.Date;
+import java.util.Map;
+import java.util.function.Supplier;
+import org.apache.pulsar.client.api.Authentication;
+import org.apache.pulsar.client.api.BatcherBuilder;
+import org.apache.pulsar.client.api.ClientBuilder;
+import org.apache.pulsar.client.api.MessageId;
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.api.Schema;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericSchema;
+import org.apache.pulsar.client.api.schema.RecordSchemaBuilder;
+import org.apache.pulsar.client.api.schema.SchemaDefinition;
+import org.apache.pulsar.client.api.schema.SchemaDefinitionBuilder;
+import org.apache.pulsar.client.internal.PulsarClientImplementationBinding;
+import org.apache.pulsar.common.schema.KeyValue;
+import org.apache.pulsar.common.schema.KeyValueEncodingType;
+import org.apache.pulsar.common.schema.SchemaInfo;
+import org.apache.pulsar.common.schema.SchemaInfoWithVersion;
+import org.apache.pulsar.common.schema.SchemaType;
+
+/**
+ * Helper class for class instantiations and it also contains methods to work with schemas.
+ */
+@SuppressWarnings("unchecked")
+public class PulsarClientImplementationBindingImpl implements PulsarClientImplementationBinding {
+
+ public <T> SchemaDefinitionBuilder<T> newSchemaDefinitionBuilder() {
+ return new org.apache.pulsar.client.impl.schema.SchemaDefinitionBuilderImpl();
+ }
+
+ public ClientBuilder newClientBuilder() {
+ return new org.apache.pulsar.client.impl.ClientBuilderImpl();
Review comment:
Got it. thanks!
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #11636: Java Client: remove usage of reflection while using Pulsar Implementation classes
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #11636:
URL: https://github.com/apache/pulsar/pull/11636#discussion_r687172955
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java
##########
@@ -0,0 +1,348 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.impl;
+
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.charset.Charset;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.util.Date;
+import java.util.Map;
+import java.util.function.Supplier;
+import org.apache.pulsar.client.api.Authentication;
+import org.apache.pulsar.client.api.BatcherBuilder;
+import org.apache.pulsar.client.api.ClientBuilder;
+import org.apache.pulsar.client.api.MessageId;
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.api.Schema;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericSchema;
+import org.apache.pulsar.client.api.schema.RecordSchemaBuilder;
+import org.apache.pulsar.client.api.schema.SchemaDefinition;
+import org.apache.pulsar.client.api.schema.SchemaDefinitionBuilder;
+import org.apache.pulsar.client.internal.PulsarClientImplementationBinding;
+import org.apache.pulsar.common.schema.KeyValue;
+import org.apache.pulsar.common.schema.KeyValueEncodingType;
+import org.apache.pulsar.common.schema.SchemaInfo;
+import org.apache.pulsar.common.schema.SchemaInfoWithVersion;
+import org.apache.pulsar.common.schema.SchemaType;
+
+/**
+ * Helper class for class instantiations and it also contains methods to work with schemas.
+ */
+@SuppressWarnings("unchecked")
+public class PulsarClientImplementationBindingImpl implements PulsarClientImplementationBinding {
+
+ public <T> SchemaDefinitionBuilder<T> newSchemaDefinitionBuilder() {
+ return new org.apache.pulsar.client.impl.schema.SchemaDefinitionBuilderImpl();
+ }
+
+ public ClientBuilder newClientBuilder() {
+ return new org.apache.pulsar.client.impl.ClientBuilderImpl();
Review comment:
Wouldn't this be using the class loader from the current context? How can we be sure it's going to be the same as `PulsarClientImplementationBindingImpl`?
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java
##########
@@ -0,0 +1,348 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.impl;
+
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.charset.Charset;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.util.Date;
+import java.util.Map;
+import java.util.function.Supplier;
+import org.apache.pulsar.client.api.Authentication;
+import org.apache.pulsar.client.api.BatcherBuilder;
+import org.apache.pulsar.client.api.ClientBuilder;
+import org.apache.pulsar.client.api.MessageId;
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.api.Schema;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericSchema;
+import org.apache.pulsar.client.api.schema.RecordSchemaBuilder;
+import org.apache.pulsar.client.api.schema.SchemaDefinition;
+import org.apache.pulsar.client.api.schema.SchemaDefinitionBuilder;
+import org.apache.pulsar.client.internal.PulsarClientImplementationBinding;
+import org.apache.pulsar.common.schema.KeyValue;
+import org.apache.pulsar.common.schema.KeyValueEncodingType;
+import org.apache.pulsar.common.schema.SchemaInfo;
+import org.apache.pulsar.common.schema.SchemaInfoWithVersion;
+import org.apache.pulsar.common.schema.SchemaType;
+
+/**
+ * Helper class for class instantiations and it also contains methods to work with schemas.
+ */
+@SuppressWarnings("unchecked")
+public class PulsarClientImplementationBindingImpl implements PulsarClientImplementationBinding {
+
+ public <T> SchemaDefinitionBuilder<T> newSchemaDefinitionBuilder() {
+ return new org.apache.pulsar.client.impl.schema.SchemaDefinitionBuilderImpl();
+ }
+
+ public ClientBuilder newClientBuilder() {
+ return new org.apache.pulsar.client.impl.ClientBuilderImpl();
+ }
+
+ public MessageId newMessageId(long ledgerId, long entryId, int partitionIndex) {
+ return new org.apache.pulsar.client.impl.MessageIdImpl(ledgerId, entryId, partitionIndex);
Review comment:
Couldn't we import the classes here instead of using the full name?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #11636: Java Client: remove usage of reflection while using Pulsar Implementation classes
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11636:
URL: https://github.com/apache/pulsar/pull/11636#discussion_r687509897
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java
##########
@@ -0,0 +1,348 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.impl;
+
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.charset.Charset;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.util.Date;
+import java.util.Map;
+import java.util.function.Supplier;
+import org.apache.pulsar.client.api.Authentication;
+import org.apache.pulsar.client.api.BatcherBuilder;
+import org.apache.pulsar.client.api.ClientBuilder;
+import org.apache.pulsar.client.api.MessageId;
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.api.Schema;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericSchema;
+import org.apache.pulsar.client.api.schema.RecordSchemaBuilder;
+import org.apache.pulsar.client.api.schema.SchemaDefinition;
+import org.apache.pulsar.client.api.schema.SchemaDefinitionBuilder;
+import org.apache.pulsar.client.internal.PulsarClientImplementationBinding;
+import org.apache.pulsar.common.schema.KeyValue;
+import org.apache.pulsar.common.schema.KeyValueEncodingType;
+import org.apache.pulsar.common.schema.SchemaInfo;
+import org.apache.pulsar.common.schema.SchemaInfoWithVersion;
+import org.apache.pulsar.common.schema.SchemaType;
+
+/**
+ * Helper class for class instantiations and it also contains methods to work with schemas.
+ */
+@SuppressWarnings("unchecked")
+public class PulsarClientImplementationBindingImpl implements PulsarClientImplementationBinding {
+
+ public <T> SchemaDefinitionBuilder<T> newSchemaDefinitionBuilder() {
+ return new org.apache.pulsar.client.impl.schema.SchemaDefinitionBuilderImpl();
+ }
+
+ public ClientBuilder newClientBuilder() {
+ return new org.apache.pulsar.client.impl.ClientBuilderImpl();
+ }
+
+ public MessageId newMessageId(long ledgerId, long entryId, int partitionIndex) {
+ return new org.apache.pulsar.client.impl.MessageIdImpl(ledgerId, entryId, partitionIndex);
Review comment:
Fixed.
I left the FQCN only because I converted the class semi automatically, before of this change they where simple strings.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #11636: Java Client: remove usage of reflection while using Pulsar Implementation classes
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11636:
URL: https://github.com/apache/pulsar/pull/11636#discussion_r687529763
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImplementationBindingImpl.java
##########
@@ -0,0 +1,348 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.impl;
+
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.charset.Charset;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.util.Date;
+import java.util.Map;
+import java.util.function.Supplier;
+import org.apache.pulsar.client.api.Authentication;
+import org.apache.pulsar.client.api.BatcherBuilder;
+import org.apache.pulsar.client.api.ClientBuilder;
+import org.apache.pulsar.client.api.MessageId;
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.api.Schema;
+import org.apache.pulsar.client.api.schema.GenericRecord;
+import org.apache.pulsar.client.api.schema.GenericSchema;
+import org.apache.pulsar.client.api.schema.RecordSchemaBuilder;
+import org.apache.pulsar.client.api.schema.SchemaDefinition;
+import org.apache.pulsar.client.api.schema.SchemaDefinitionBuilder;
+import org.apache.pulsar.client.internal.PulsarClientImplementationBinding;
+import org.apache.pulsar.common.schema.KeyValue;
+import org.apache.pulsar.common.schema.KeyValueEncodingType;
+import org.apache.pulsar.common.schema.SchemaInfo;
+import org.apache.pulsar.common.schema.SchemaInfoWithVersion;
+import org.apache.pulsar.common.schema.SchemaType;
+
+/**
+ * Helper class for class instantiations and it also contains methods to work with schemas.
+ */
+@SuppressWarnings("unchecked")
+public class PulsarClientImplementationBindingImpl implements PulsarClientImplementationBinding {
+
+ public <T> SchemaDefinitionBuilder<T> newSchemaDefinitionBuilder() {
+ return new org.apache.pulsar.client.impl.schema.SchemaDefinitionBuilderImpl();
+ }
+
+ public ClientBuilder newClientBuilder() {
+ return new org.apache.pulsar.client.impl.ClientBuilderImpl();
Review comment:
See the JVM specs here
https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-5.html
chapter "5.3. Creation and Loading"
look for "If D was defined by a user-defined class loader, then that same user-defined class loader initiates loading of C"
basically if a class "D" (where "D" is not a system class) triggers the loading of a class "C", then the same Classloader that loaded "D" will initiate the loading of "C"
So in this case the same classloader which loaded `PulsarClientImplementationBindingImpl` will initiate the loading of `ClientBuilderImpl` if the class has not been loaded yet
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11636: Java Client: remove usage of reflection while using Pulsar Implementation classes
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11636:
URL: https://github.com/apache/pulsar/pull/11636#issuecomment-897475256
@merlimat I have removed the usage of Fully qualified class names.
I have answered to your question about classloading (I had to verify this in the specs).
After merging this patch I will add new integration tests in Pulsar Functions in order to verify that we are loading correctly all (of most of them) the classes referred by `PulsarClientImplementationBindingImpl` (Schema.XXX, RecordSchemaBuilder, BatcherBuilder, KeyValueSchema....)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11636: Java Client: remove usage of reflection while using Pulsar Implementation classes
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11636:
URL: https://github.com/apache/pulsar/pull/11636#issuecomment-897456337
> @eolivelli thanks for providing doc-related info. If your PR does not need to update doc, could you please help label the PR with `no-need-doc` in the future? Thanks
sure. thanks for the reminder
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #11636: Java Client: remove usage of reflection while using Pulsar Implementation classes
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #11636:
URL: https://github.com/apache/pulsar/pull/11636
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #11636: Java Client: remove usage of reflection while using Pulsar Implementation classes
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #11636:
URL: https://github.com/apache/pulsar/pull/11636#issuecomment-897248720
@eolivelli thanks for providing doc-related info. If your PR does not need to update doc, could you please help label the PR with `no-need-doc` in the future? Thanks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org