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 2022/06/08 10:03:25 UTC
[GitHub] [pulsar] Nicklee007 opened a new pull request, #15979: [fix][broker] The DEFAULT_NAR_EXTRACTION_DIR will cause NoClassDefFoundError
Nicklee007 opened a new pull request, #15979:
URL: https://github.com/apache/pulsar/pull/15979
Fixes #15978
### Motivation
There's an issue reported about problems in load protocol handler .class file. This PR will specify default tmp dir;
### Modifications
Specify default tmp dir by `-Djava.io.tmpdir=$PULSAR_HOME/tmp`
- [X] `doc-not-needed`
--
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] Nicklee007 commented on pull request #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
Nicklee007 commented on PR #15979:
URL: https://github.com/apache/pulsar/pull/15979#issuecomment-1152054894
@michaeljmarshall @gaozhangmin update `DEFAULT_NAR_EXTRACTION_DIR` default value ,which will choice `$PULSAR_HOME/tmp` dir by default, also user can specify the nar extraction dir by configure `narExtractionDirectory` in conf/broker.conf. PTAL
--
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] Nicklee007 commented on a diff in pull request #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
Nicklee007 commented on code in PR #15979:
URL: https://github.com/apache/pulsar/pull/15979#discussion_r895301466
##########
bin/pulsar:
##########
@@ -290,6 +290,9 @@ if [[ -z "$IS_JAVA_8" ]]; then
OPTS="$OPTS --add-opens java.base/sun.net=ALL-UNNAMED --add-opens java.management/sun.management=ALL-UNNAMED"
fi
+# Specify default nar tmp dir
+OPTS="$OPTS -Dnar.extraction.tmpdir=$PULSAR_HOME/tmp"
Review Comment:
Default use the /tmp dir has a risk which will cause the class file be auto cleaned by os, when the class file
used again , the class not find error occur . Now when the `nar.extraction.tmpdir` not configured will use the old config `java.io.tmpdir` , so the change will not break existing users again .
--
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] Nicklee007 commented on a diff in pull request #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
Nicklee007 commented on code in PR #15979:
URL: https://github.com/apache/pulsar/pull/15979#discussion_r895300148
##########
pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarClassLoader.java:
##########
@@ -139,7 +139,7 @@ public class NarClassLoader extends URLClassLoader {
private static final String TMP_DIR_PREFIX = "pulsar-nar";
- public static final String DEFAULT_NAR_EXTRACTION_DIR = System.getProperty("java.io.tmpdir");
+ public static final String DEFAULT_NAR_EXTRACTION_DIR = System.getProperty("nar.extraction.tmpdir");
Review Comment:
Has changed, if `nar.extraction.tmpdir` not configured will use the old config` java.io.tmpdir`.
--
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 diff in pull request #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #15979:
URL: https://github.com/apache/pulsar/pull/15979#discussion_r894331214
##########
pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarClassLoader.java:
##########
@@ -139,7 +139,7 @@ public class NarClassLoader extends URLClassLoader {
private static final String TMP_DIR_PREFIX = "pulsar-nar";
- public static final String DEFAULT_NAR_EXTRACTION_DIR = System.getProperty("java.io.tmpdir");
+ public static final String DEFAULT_NAR_EXTRACTION_DIR = System.getProperty("nar.extraction.tmpdir");
Review Comment:
You have to fallback on the legacy name if this property is not configured
##########
bin/pulsar:
##########
@@ -290,6 +290,9 @@ if [[ -z "$IS_JAVA_8" ]]; then
OPTS="$OPTS --add-opens java.base/sun.net=ALL-UNNAMED --add-opens java.management/sun.management=ALL-UNNAMED"
fi
+# Specify default nar tmp dir
+OPTS="$OPTS -Dnar.extraction.tmpdir=$PULSAR_HOME/tmp"
Review Comment:
Please don't change this default behaviour, we are going to break existing users that upgrade.
Now that you hBe the system property you can let your users configure it
--
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] Nicklee007 commented on pull request #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
Nicklee007 commented on PR #15979:
URL: https://github.com/apache/pulsar/pull/15979#issuecomment-1160259965
> There is actually now way to express that.
> You can create a new issue to change the default value, and wait for 2.12 to start.
Thx for your suggestion, I have cancel to change the default nar tmp dir to `$PULSAR_HOME/tmp` in this PR. The relate PR #16143 will add in other release like 2.12.
Now, the PR PTAL again, Thx.
--
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] Nicklee007 commented on pull request #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
Nicklee007 commented on PR #15979:
URL: https://github.com/apache/pulsar/pull/15979#issuecomment-1151851985
> Would it make more sense to update the default for `DEFAULT_NAR_EXTRACTION_DIR` instead of changing the `java.io.tmpdir` location?
@michaeljmarshall @gaozhangmin Update the `DEFAULT_NAR_EXTRACTION_DIR` has smaller effect than change `java.io.tmpdir` ,I will change the default value of `DEFAULT_NAR_EXTRACTION_DIR`. Anyway default use the /tmp dir is not a good choice, the dir at risk be cleaned by os.
--
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 diff in pull request #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #15979:
URL: https://github.com/apache/pulsar/pull/15979#discussion_r895546097
##########
bin/pulsar:
##########
@@ -290,6 +290,9 @@ if [[ -z "$IS_JAVA_8" ]]; then
OPTS="$OPTS --add-opens java.base/sun.net=ALL-UNNAMED --add-opens java.management/sun.management=ALL-UNNAMED"
fi
+# Specify default nar tmp dir
+OPTS="$OPTS -Dnar.extraction.tmpdir=$PULSAR_HOME/tmp"
Review Comment:
there is no guarantee that $PULSAR_HOME/tmp exists and it is writable by the user.
if we change the behaviour then we have to document it, but, as a general rule, we cannot break things during upgrades.
it is better to add this feature, in 2.11, then we can change the default behaviour in 2.12.
also, in linux /tmp is not emptied randomly, my experience it that such directory is empty at boot
--
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 merged pull request #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
eolivelli merged PR #15979:
URL: https://github.com/apache/pulsar/pull/15979
--
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] gaozhangmin commented on pull request #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #15979:
URL: https://github.com/apache/pulsar/pull/15979#issuecomment-1151817753
> DEFAULT_NAR_EXTRACTION_DIR
+1, we can config it through `narExtractionDirectory`
--
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 #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #15979:
URL: https://github.com/apache/pulsar/pull/15979#issuecomment-1155071695
> Could you help me add some label to mark the feature will add in the follow release, Thx!
There is actually now way to express that.
You can create a new issue to change the default value, and wait for 2.12 to start.
This is a OSS project, so it is hard to make plans for the future, as we are all volounteers.
I suggest you to create the issue and sponsor it when it will be the time.
I can't think of a better way currently
@merlimat do you have some better suggestion ? like creating 2.12 ahead of time ?
--
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] Nicklee007 commented on a diff in pull request #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
Nicklee007 commented on code in PR #15979:
URL: https://github.com/apache/pulsar/pull/15979#discussion_r896507302
##########
bin/pulsar:
##########
@@ -290,6 +290,9 @@ if [[ -z "$IS_JAVA_8" ]]; then
OPTS="$OPTS --add-opens java.base/sun.net=ALL-UNNAMED --add-opens java.management/sun.management=ALL-UNNAMED"
fi
+# Specify default nar tmp dir
+OPTS="$OPTS -Dnar.extraction.tmpdir=$PULSAR_HOME/tmp"
Review Comment:
> it is better to add this feature, in 2.11, then we can change the default behaviour in 2.12.
Thx for the Suggestions.
> also, in linux /tmp is not emptied randomly, my experience it that such directory is empty at boot
maybe diff linux os has diff behaviours for the /tmp dir clean, Debian will clean when the boot , but as Centos or RedHat will auto clean the file in the /tmp dir when 7 days or 30 days the file not be used, In this way, the .class file will loss.
In our case, we add the KOP protocol, after few days when we want use the KOP protocol, we find some .class have been cleaned by os.
--
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] Nicklee007 commented on pull request #15979: [fix][broker] fix DEFAULT_NAR_EXTRACTION_DIR cause NoClassDefFoundError
Posted by GitBox <gi...@apache.org>.
Nicklee007 commented on PR #15979:
URL: https://github.com/apache/pulsar/pull/15979#issuecomment-1155058064
> I support the change, as far as we don't change the behaviour at least for this release.
@eolivelli Could you help me add some label to mark the feature will add in the follow release, Thx!
--
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