Add unified way of initializing classes via string and configuring them. #943

FelixEngl · 2022-02-03T17:04:11Z

I extracted the code for initializing classes from string and configuring them from my fork https://github.com/FelixEngl/storm-crawler/tree/local_version. (This branch contains all fixes/changes that i made. I'll try to extract PR after PR until both are on a same level.)

I don't know how far you got with #937, but I this one has a more reasonable size with the trade-off that it introduces some new warnings due to missing @Contract, @NotNull and @Nullable annotations in various sub-classes and sub-interfaces. These warnings will be fixed by either #937 or a PR in the future.

Best Regards

Felix

Signed-off-by: Felix Engl felix.engl@hotmail.com

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

sonatype-lift · 2022-02-03T17:25:10Z

core/src/main/java/com/digitalpebble/stormcrawler/persistence/urlbuffer/URLBuffer.java


+    /** Replace with URLBufferUtil.createInstance */
+    @Deprecated
+    public static URLBuffer getInstance(Map<String, Object> stormConf) {


InlineMeSuggester: This deprecated API looks inlineable. If you'd like the body of the API to be inlined to its callers, please annotate it with @InlineMe. (details)
(at-me in a reply with help or ignore)

sonatype-lift · 2022-02-03T17:25:11Z

core/src/main/java/com/digitalpebble/stormcrawler/util/Configurable.java

-     */
-    @SuppressWarnings("rawtypes")
+    /** @deprecated Replace with ConfigurableUtil.configure */
+    @Deprecated
    public static <T extends Configurable> List<T> configure(


InlineMeSuggester: This deprecated API looks inlineable. If you'd like the body of the API to be inlined to its callers, please annotate it with @InlineMe. (details)
(at-me in a reply with help or ignore)

sonatype-lift · 2022-02-03T17:25:15Z

core/src/main/java/com/digitalpebble/stormcrawler/util/Configurable.java


+/** @see ConfigurableUtil for more information */


MissingSummary: A summary fragment is required; consider using the value of the @see block as a summary fragment instead. (details)
(at-me in a reply with help or ignore)

jnioche · 2022-02-04T08:47:41Z

hi @FelixEngl did you see my comments in #937? Some of the changes you are proposing here would meet the same objections e.g. use of org.jetbrains annotations.

FelixEngl · 2022-02-04T20:22:17Z

hi @FelixEngl did you see my comments in #937? Some of the changes you are proposing here would meet the same objections e.g. use of org.jetbrains annotations.

No, where are these comments? I didn't see them in that PR-Conversation and I didn't get any kind of notification. Or are there any other tabs that I have to check for notifications like that?
That is my first time working with other people soely with GitHub, usually I'm working alone or in little groups of 5-7 people in the same room. So using GitHub for Code-Reviews is quite new to me.
(Maybe I messed up with my notification-settings? I'll check them up later.)

jnioche · 2022-02-05T08:38:33Z

No, where are these comments? I didn't see them in that PR-Conversation and I didn't get any kind of notification. Or are there any other tabs that I have to check for notifications like that?

They were pending, you should be able to see them now

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

jnioche · 2022-02-08T13:49:44Z

No rest for the wicked ;-)

Could you please explain what you are trying to achieve in the description of this PR and why you think it is necessary?

FelixEngl · 2022-02-08T14:07:19Z

No rest for the wicked ;-)

Could you please explain what you are trying to achieve in the description of this PR and why you think it is necessary?

Yes, the next one will be a little bit better with the description. I didn't know it better until recently.
But I learned a lot from our discussions in #937. Thanks again for that opportunity and your patience! :)

Basically:
A lot of classes of SC use some kind of initialisation where they initialize a class by string name. But all of them just copy-paste the very same code or a very similar one. Some of the copied codes are fautly or at least error prone without any useful information for the user/developer when they fail like "failed to initialize".

This PR proposes a unified toolkit for initializing classes from qualified names. The core of this PR is the InitialisationUtil-class.

Proposing a universal way has multiple advantages:

No more copy paste code.
A clean and direct approach that works for everyone.
If the way how a class is initialized by string changes, we can simply adapt the change at a single place and not everywhere.
The InitialisationUtil is even useable from developers using SC, therefore writing clean code/extensions by utilizing this for initializing from string names leads to a more secure and clean code in the future.
Due to the importance of that feature I decided to create fine grained exceptions with easy to understand messages, making the development/debugging for and with SC much more easier.
The new InitialisationUtil even allows for additional checks to assert, that the initialized class does not just implement the said superclass but also some other interface.

sonatype-lift · 2022-02-08T14:07:59Z

core/src/main/java/com/digitalpebble/stormcrawler/protocol/DelegatorProtocol.java

@@ -156,19 +144,27 @@ boolean isMatch(final Metadata metadata) {
    private final LinkedList<FilteredProtocol> protocols = new LinkedList<>();


JdkObsolete: It is very rare for LinkedList to out-perform ArrayList or ArrayDeque. Avoid it unless you're willing to invest a lot of time into benchmarking. Caveat: LinkedList supports null elements, but ArrayDeque does not. (details)
(at-me in a reply with help or ignore)

@FelixEngl could we replace the LinkedList with an ArrayList or ArrayDeque as suggested by Lift?

FelixEngl · 2022-02-08T14:14:40Z

@jnioche Ah, I forgot something:
The other notable change is at core/src/main/java/com/digitalpebble/stormcrawler/protocol/ProtocolFactory.java where I introduce double checket locking for a more secure access of the singleton in a multithreaded context.
Otherwise there could be cases where multiple instances of the singleton are created.

rzo1 · 2022-02-08T14:17:36Z

core/src/main/java/com/digitalpebble/stormcrawler/parse/JSoupFilters.java

-     *
-     * @throws IOException
-     */
+    /** loads the filters from a JSON configuration file */


This might break end user code (removal of a checked exception), so we need to be clear in the changelog.

Why should this break end user code?
It still throws that IOException but i removed the empty comment.

rzo1 · 2022-02-08T14:18:31Z

core/src/main/java/com/digitalpebble/stormcrawler/persistence/Scheduler.java

        } catch (Exception e) {
-            throw new RuntimeException("Can't instanciate " + className);
+            throw new RuntimeException("Can't instanciate " + className, e);


Good catch!

rzo1 · 2022-02-08T14:19:53Z

core/src/main/java/com/digitalpebble/stormcrawler/persistence/urlbuffer/URLBuffer.java

@@ -85,29 +89,4 @@ default void acked(String url) {
    }

    default void configure(Map<String, Object> stormConf) {}
-
-    /** Returns a URLBuffer instance based on the configuration * */
-    public static URLBuffer getInstance(Map stormConf) {


This might break end user code (removal of a public method), so we need to be clear in the changelog.

I deprecated it at line 42.
I just moved it up. Strange that git doesn't recognize this. :/

rzo1 · 2022-02-08T14:24:11Z

core/src/main/java/com/digitalpebble/stormcrawler/util/ConfigurableUtil.java

@@ -0,0 +1,128 @@
+package com.digitalpebble.stormcrawler.util;


Misses a license header

yes every new class needs a license header

rzo1 · 2022-02-08T14:25:05Z

...ble/stormcrawler/util/exceptions/initialisation/ClassForInitialisationNotFoundException.java

@@ -0,0 +1,7 @@
+package com.digitalpebble.stormcrawler.util.exceptions.initialisation;


License? wdyt @jnioche

yes every new class needs a license header

@jnioche What about Tests?

yes, test classes as well please.

rzo1 · 2022-02-08T14:27:16Z

core/src/test/java/com/digitalpebble/stormcrawler/helper/initialisation/SimpleOpenClass.java

@@ -0,0 +1,3 @@
+package com.digitalpebble.stormcrawler.helper.initialisation;


License? wdyt @jnioche

rzo1 · 2022-02-08T14:27:25Z

core/src/test/java/com/digitalpebble/stormcrawler/helper/initialisation/base/AbstractClass.java

@@ -0,0 +1,21 @@
+package com.digitalpebble.stormcrawler.helper.initialisation.base;


License? wdyt @jnioche

rzo1 · 2022-02-08T14:27:56Z

.../src/test/java/com/digitalpebble/stormcrawler/helper/initialisation/base/ITestInterface.java

@@ -0,0 +1,3 @@
+package com.digitalpebble.stormcrawler.helper.initialisation.base;


License? wdyt @jnioche

rzo1 · 2022-02-08T14:28:00Z

...alpebble/stormcrawler/helper/initialisation/base/OpenClassWithAbstractClassAndInterface.java

@@ -0,0 +1,4 @@
+package com.digitalpebble.stormcrawler.helper.initialisation.base;


License? wdyt @jnioche

rzo1 · 2022-02-08T14:28:09Z

core/src/test/java/com/digitalpebble/stormcrawler/util/InitialisationUtilTest.java

@@ -0,0 +1,168 @@
+package com.digitalpebble.stormcrawler.util;


License? wdyt @jnioche

jnioche · 2022-02-08T14:47:48Z

Guys, apologies in advance if I come across as being a bit blunt.
I am totally in favour of improving aspect of the code but this will not come at the cost of adding I-don't-know-how-many classes for various exceptions + tons if utility classes and so on, for what is not a new functionality that users will benefit from or a major improvement.
One of the things that I am pleased to have achieved with SC is (I think) a good trade-off between code simplicity and richness of features. I intend to keep it that way.
Can this be done in a way that doesn't involve the addition of so many new classes?
Thanks

FelixEngl · 2022-02-08T14:52:48Z

Guys, apologies in advance if I come across as being a bit blunt. I am totally in favour of improving aspect of the code but this will not come at the cost of adding I-don't-know-how-many classes for various exceptions + tons if utility classes and so on, for what is not a new functionality that users will benefit from or a major improvement. One of the things that I am pleased to have achieved with SC is (I think) a good trade-off between code simplicity and richness of features. I intend to keep it that way. Can this be done in a way that doesn't involve the addition of so many new classes? Thanks

Don't worry about being blunt, I prefer it when someone speaks his/her mind directly instead of dancing around the problem or finding some straw arguments. :D

I could remove the exception classes and only throw runtime exceptions. That would reduce the number of classes.
Would that be enough or are there other classes that are too much too?

rzo1 · 2022-02-08T14:59:03Z

I think, that it looks like a lot of classes. Basically, I see 2 (new) utility classes + a lot of test-related classes + (runtime) exception sugar. Guess, that the new runtime exceptions can be dropped.

FelixEngl · 2022-02-08T15:00:49Z

I think, that it looks like a lot of classes. Basically, I see 2 (new) utility classes + a lot of test-related classes + (runtime) exception sugar. Guess, that the new runtime exceptions can be dropped.

Then I'll drop them.
And you are right. Basically we have just 2 new Utility Classes and a lot of tests.
I wanted to test that feature very thoroughly because it's very often used.

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

FelixEngl · 2022-02-08T15:20:07Z

@jnioche I removed the Exceptions. We could also remove the URLBufferUtil if you want.
Then we would basically only introduce two new classes in the code.

But we need the other new classes for testing the initialisation feature, so we can't remove the new classes created in the test source.

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

# Conflicts: # core/src/main/java/com/digitalpebble/stormcrawler/persistence/urlbuffer/URLBuffer.java

FelixEngl · 2022-02-08T15:48:19Z

@jnioche I removed the Exceptions. We could also remove the URLBufferUtil if you want. Then we would basically only introduce one new class in the code.

But we need the other new classes for testing the initialisation feature, so we can't remove the new classes created in the test source.

Removed the URLBufferUtil too, now we have only two new classes. InitialisationUtil and ConfigurableUtil.

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

sonatype-lift · 2022-02-08T15:57:39Z

core/src/main/java/com/digitalpebble/stormcrawler/persistence/urlbuffer/URLBuffer.java

+
+    /** Replace with {@link URLBuffer#createInstance(Map)} */
+    @Deprecated
+    static URLBuffer getInstance(Map<String, Object> stormConf) {


InlineMeSuggester: This deprecated API looks inlineable. If you'd like the body of the API to be inlined to its callers, please annotate it with @InlineMe. (details)
(at-me in a reply with help or ignore)

sonatype-lift · 2022-02-08T16:15:49Z

core/src/main/java/com/digitalpebble/stormcrawler/util/Configurable.java

-        return filterLists;
+    /** @deprecated Replace with ConfigurableUtil.configure */
+    @Deprecated
+    static <T extends Configurable> List<T> configure(


InlineMeSuggester: This deprecated API looks inlineable. If you'd like the body of the API to be inlined to its callers, please annotate it with @InlineMe. (details)
(at-me in a reply with help or ignore)

jnioche · 2022-02-09T11:22:28Z

@jnioche I removed the Exceptions. We could also remove the URLBufferUtil if you want. Then we would basically only introduce one new class in the code.
But we need the other new classes for testing the initialisation feature, so we can't remove the new classes created in the test source.

Removed the URLBufferUtil too, now we have only two new classes. InitialisationUtil and ConfigurableUtil.

Great! That's way better

InitialisationUtil has no equivalent in the current code, so we definitely need it. ConfigurableUtil is a bit different. I see that you removed the utility code from the interface and kept the latter as a strict interface. I can understand why but to be honest I quite liked having the utility code within the interface class - all in one place. Probably not orthodox, I agree but I thought it was quite elegant in a way. How open would you be to keeping it that way? This is just an extra class, I can also be swayed.

And @rzo1 is right, we need license headers on every new file.

Thanks for your open-mindedness BTW.

jnioche · 2022-02-09T11:26:21Z

core/src/main/java/com/digitalpebble/stormcrawler/protocol/ProtocolFactory.java


    public static ProtocolFactory getInstance(Config conf) {

-        if (single_instance != null) return single_instance;
+        // https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java


such a great bugfix! thanks for that

FelixEngl · 2022-02-09T11:27:32Z

@jnioche I removed the Exceptions. We could also remove the URLBufferUtil if you want. Then we would basically only introduce one new class in the code.
But we need the other new classes for testing the initialisation feature, so we can't remove the new classes created in the test source.

Removed the URLBufferUtil too, now we have only two new classes. InitialisationUtil and ConfigurableUtil.

Great! That's way better

InitialisationUtil has no equivalent in the current code, so we definitely need it. ConfigurableUtil is a bit different. I see that you removed the utility code from the interface and kept the latter as a strict interface. I can understand why but to be honest I quite liked having the utility code within the interface class - all in one place. Probably not orthodox, I agree but I thought it was quite elegant in a way. How open would you be to keeping it that way? This is just an extra class, I can also be swayed.

And @rzo1 is right, we need license headers on every new file.

Thanks for your open-mindedness BTW.

I didn't have a problem with the static function (tbh I like putting creation calls for interfaces there too) but i had a problem with the static LOG reference in the interface. That was used by one of subclasses, and that is quite confusing when logging and you only see the interface-reference.

What about hiding the ConfigUtil in package scope and keeping the original creation method in the interface?
That way we can assure that the Logger for the configure is only called by the Configure and nowhere else.

…tionUtil to ParserBolt. Use ArrayList in DelegatorProtocol. Signed-off-by: Felix Engl <felix.engl@hotmail.com>

FelixEngl · 2022-02-09T13:21:56Z

@jnioche I implemented the hidden-util-class approach, I think this should make both of us happy.
You are/were right, it feels more natural when you use a static function in the interface for that instead of an util-class. :)
Furthermore I added to all files the license and replaced the LinkedList.

jnioche

Looks good, just one minor point about returning an immutable List

jnioche · 2022-02-09T14:02:24Z

external/tika/src/main/java/com/digitalpebble/stormcrawler/tika/ParserBolt.java

@@ -366,7 +363,7 @@ public void declareOutputFields(OutputFieldsDeclarer declarer) {
            // components check whether the URL is valid
            LOG.error("MalformedURLException on {}", parentURL);
            eventCounter.scope("error_invalid_source_url").incrBy(1);
-            return new LinkedList<Outlink>();
+            return Collections.emptyList();


the list can get things added to it later. it must not be immutable

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

Add unified way of initializing classes via string and configuring it.

e32b35e

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

sonatype-lift bot reviewed Feb 3, 2022

View reviewed changes

FelixEngl added 4 commits February 8, 2022 14:33

Merge branch 'master' into configurable

68be254

Do some merges for fest of master post apache#937

e18574c

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

Do some merges for fest of master post apache#937

4de0662

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

Merge remote-tracking branch 'origin/configurable' into configurable

8360fcc

sonatype-lift bot reviewed Feb 8, 2022

View reviewed changes

Merge branch 'DigitalPebble:master' into configurable

c48e098

rzo1 reviewed Feb 8, 2022

View reviewed changes

Remove excessive exceptions.

4f29680

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

FelixEngl added 3 commits February 8, 2022 16:46

Remove URLBufferUtil

0ef119b

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

Remove URLBufferUtil

5afbdf7

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

Merge remote-tracking branch 'origin/configurable' into configurable

f2f3868

# Conflicts: # core/src/main/java/com/digitalpebble/stormcrawler/persistence/urlbuffer/URLBuffer.java

Add NavigationFilter to Configurable family.

4381ba3

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

sonatype-lift bot reviewed Feb 8, 2022

View reviewed changes

jnioche added this to the 2.3 milestone Feb 9, 2022

jnioche added the enhancement label Feb 9, 2022

jnioche self-assigned this Feb 9, 2022

jnioche reviewed Feb 9, 2022

View reviewed changes

Add licenses to classes, rename+hide ConfigurableUtil, Add Initialisa…

7e0249f

…tionUtil to ParserBolt. Use ArrayList in DelegatorProtocol. Signed-off-by: Felix Engl <felix.engl@hotmail.com>

Merge branch 'DigitalPebble:master' into configurable

2adefa5

jnioche requested changes Feb 9, 2022

View reviewed changes

Replace Collections.emptyList() with ArrayList

d4e9719

Signed-off-by: Felix Engl <felix.engl@hotmail.com>

jnioche approved these changes Feb 9, 2022

View reviewed changes

jnioche merged commit fd2d4dc into apache:master Feb 9, 2022

FelixEngl deleted the configurable branch February 9, 2022 16:21

		@@ -156,19 +144,27 @@ boolean isMatch(final Metadata metadata) {
		private final LinkedList<FilteredProtocol> protocols = new LinkedList<>();

		@@ -0,0 +1,128 @@
		package com.digitalpebble.stormcrawler.util;

		@@ -0,0 +1,7 @@
		package com.digitalpebble.stormcrawler.util.exceptions.initialisation;

		@@ -0,0 +1,3 @@
		package com.digitalpebble.stormcrawler.helper.initialisation;

		@@ -0,0 +1,21 @@
		package com.digitalpebble.stormcrawler.helper.initialisation.base;

		@@ -0,0 +1,4 @@
		package com.digitalpebble.stormcrawler.helper.initialisation.base;

		@@ -0,0 +1,168 @@
		package com.digitalpebble.stormcrawler.util;

Add unified way of initializing classes via string and configuring them. #943

Add unified way of initializing classes via string and configuring them. #943

Uh oh!

Conversation

FelixEngl commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Developer Certificate of Origin

Uh oh!

sonatype-lift bot Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

jnioche commented Feb 4, 2022

Uh oh!

FelixEngl commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnioche commented Feb 5, 2022

Uh oh!

jnioche commented Feb 8, 2022

Uh oh!

FelixEngl commented Feb 8, 2022

Uh oh!

sonatype-lift bot Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FelixEngl commented Feb 8, 2022

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!

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!

jnioche commented Feb 8, 2022

Uh oh!

FelixEngl commented Feb 8, 2022

Uh oh!

rzo1 commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FelixEngl commented Feb 8, 2022

Uh oh!

FelixEngl commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FelixEngl commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

FelixEngl commented Feb 3, 2022 •

edited

Loading

FelixEngl commented Feb 4, 2022 •

edited

Loading

rzo1 commented Feb 8, 2022 •

edited

Loading

FelixEngl commented Feb 8, 2022 •

edited

Loading

FelixEngl commented Feb 8, 2022 •

edited

Loading

FelixEngl commented Feb 9, 2022 •

edited

Loading

FelixEngl commented Feb 9, 2022 •

edited

Loading