You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "amarkevich (via GitHub)" <gi...@apache.org> on 2023/08/21 13:41:44 UTC
[GitHub] [activemq-artemis] amarkevich opened a new pull request, #4582: ARTEMIS-4393 Explicit logger implementation instantiation
amarkevich opened a new pull request, #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582
- compilation error instead of runtime exception in case of logger annotation processor issue;
- get rid of java.lang.reflect.* & java.security.* usage for logger case
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1692040983
you're doing compile time validation.. sure.but that's a small gain compared to other things. as I said I would rather keep it this way.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1690651955
I see this interface as place where annotation can be generally placed. And log processor logic is well-known (_impl class suffix), like jpa modelgen: `The name of the metamodel class is derived from the name of the managed class by appending "_" to the name of the managed class.` Having this rule documented - its official way to instantiate logger implementation.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1690638813
It's not prohibited. just not desirable.
The dependency between the interface and its implementation is supposed to be determined at runtime by that class. If we keep the dependency explicit by just a new instance, perhaps we could just get rid of the whole processing.. but it's not something I'm looking to do at this point.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1692266650
> I listed them in jira...
The bullet points you provided in the Jira and here in the PR are _not_ a use-case. They are simply a summary of what you changed. There is no real explanation of _why_ the existing code is problematic or _how_ your change benefits the project. Perhaps those things are obvious to you, but they are not obvious to me and likely not obvious to many other developers and users.
> And still no any valuable cons
Generally speaking, the project prefers stability and continuity as that typically mitigates risk for our users - something which is greatly valued. Therefore, anybody who is proposing a change must make an understandable case for _why_ the change should be accepted.
> Aren't listed pros enough?
At this point the pros you've listed are not enough.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1686428906
What's the use-case for this? I've never had a problem with compile-time vs. runtime issues here.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1687020310
> any test
tests running during PR are green anyway
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1691694000
I still don't understand the _concrete use-case_ for this change. It seems more like [bikeshedding](https://en.wiktionary.org/wiki/bikeshedding) at the moment.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1686437016
I prefer to leave the Bundle as it was there before and not depend on the generated implementation from the interface. I prefer to keep the interface not knowing about the implementations.. which that was the real use case. (Say we could even expand the logger for new functionality).
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1686446446
@amarkevich can we keep as it was before here?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1686539693
> a problem with compile-time vs. runtime issues here
in case artemis-log-annotation-processor broken @LogBundle consumers will be built successfully but raise CNFE in runtime
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1686613926
> in case artemis-log-annotation-processor broken @LogBundle consumers will be built successfully but raise CNFE in runtime
Surely this is an uncommon event, and even if it does happen then any test or release validation will fail, right?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1690635168
Log Annotation Processor looks like any other regular annotation processors such as Lombok, Mapstruct or jpa-codegen, when generated code can be used directly in the code. There is no any reasonable points why its prohibited in this case.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1691838912
the annotation generates the implementation, and the interface must not have knowledge about the implementation, done thought the little framework.
if I could instantiate directly, we would just get rid of the processor and go straight to the implementation always.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1691860956
> interface must not have knowledge
its not mandatory. I see this interface like a centralized place where all log annotations collected
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1692016060
> than this
Aren't listed pros enough?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1691835891
Man, this is painting the bike shed.. you're not hiding the annotation..
Look how Jboss Logging used to work.. we don't want to diverge here.
If I was up to do new LoggingImplementation. I would just get rid of the annotation and use a concrete implementation directly.
and please, don't do that.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1691877917
@amarkevich Jboss Logging used to be part of this project.. and we used it in the past.. and we want to keep the same standard in this.
what's your point on this? this change is not going to happen. we can spend time into something more positive than this.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1687221889
> keep the interface not knowing about the implementations
its better to hide logger implementation - done
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1690415066
-1000 on this.. sorry!!!
The whole point of the BundleFactory is not require knowing the implementation from the Interface, and having the Factory figuring out what to do.
I don't mind about this reflection here as it's only happening once.. it's more trouble really.
the framework where this came from is Jboss Logging, where we are doing exactly what it was done before, except that we did that to fix some licensing and to be able to use SLF4J. I hate to block a PR from someone but I strongly disagree with this one man! sorry about that!
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1690942314
We don't see it that way. Interfaces shouldn't have the implementation defined in there. The factory provides somewhat an abstraction.
We can agree to disagree here.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1691857979
> you're not hiding the annotation
yep - logger implementation details are hidden
> Jboss Logging
is not a part of this project
> use a concrete implementation directly
current log processor generates message prefix general way and protect log messages from being corrupted by mistake
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1691824453
> _concrete use-case_
I listed them in jira:
- compilation error instead of runtime exception in case of logger annotation processor issue;
- get rid of java.lang.reflect.* & java.security.* usage for logger case
- hide logger implementation
And still no any valuable cons
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic closed pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic closed pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
URL: https://github.com/apache/activemq-artemis/pull/4582
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1691182583
> Interfaces shouldn't have the implementation defined in there.
So in case abstract class with annotations will be used instead of current interface - it can be approved?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] amarkevich commented on pull request #4582: ARTEMIS-4393 Explicit logger implementation instantiation
Posted by "amarkevich (via GitHub)" <gi...@apache.org>.
amarkevich commented on PR #4582:
URL: https://github.com/apache/activemq-artemis/pull/4582#issuecomment-1692456421
> prefers stability
in one of previous PR I made a mistake and broke artemis-log-annotation-processor. And PR check passed. This is my own expirience of using this project as developer. Feel free to keep such behaviour.
> why the change should be accepted
get rid of reflection is the main point
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org