Discussion:
Change in ovirt-engine[master]: aaa: extensions test tool: logger
(too old to reply)
a***@ovirt.org
2015-02-17 19:56:42 UTC
Permalink
***@ovirt.org has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-HasComments: No
Alon Bar-Lev
2015-02-17 20:12:09 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 1:

(12 comments)

http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/pom.xml
File backend/manager/extension-tool/pom.xml:

Line 39: <groupId>commons-logging</groupId>
Line 40: <artifactId>commons-logging</artifactId>
Line 41: <version>${commons-logging.version}</version>
Line 42: <scope>compile</scope>
Line 43: </dependency>
log4j please
Line 44: </dependencies>
Line 45:
Line 46: <build>
Line 47: <plugins>


http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolArguments.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolArguments.java:

Line 6: import java.util.Map;
Line 7: import java.util.Properties;
Line 8: import java.util.Set;
Line 9:
Line 10: import com.sun.org.apache.xpath.internal.operations.Mod;
what is that?
Line 11: import org.ovirt.engine.core.uutils.cli.ArgumentBuilder;
Line 12: import org.ovirt.engine.core.uutils.cli.ExtendedCliParser;
Line 13:
Line 14: public class ExtensionsToolArguments {


Line 7: import java.util.Properties;
Line 8: import java.util.Set;
Line 9:
Line 10: import com.sun.org.apache.xpath.internal.operations.Mod;
Line 11: import org.ovirt.engine.core.uutils.cli.ArgumentBuilder;
oh! you are using this one... I hate this class... but well... maybe we improve it as part of this work.
Line 12: import org.ovirt.engine.core.uutils.cli.ExtendedCliParser;
Line 13:
Line 14: public class ExtensionsToolArguments {
Line 15:


http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/Module.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/Module.java:

Line 1: package org.ovirt.engine.exttool.core;
Line 2:
Line 3: public enum Module {
this class is bad as you do not want to patch it when adding new modules, the entire idea of services is that you can dynamically detect them at runtime.
Line 4:
Line 5: AAA("aaa"),
Line 6: LOGGER("logger");
Line 7:


http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java:

Line 1: package org.ovirt.engine.exttool.core;
Line 2:
Line 3: public interface ModuleService {
Line 4:
Line 5: public void runModule(ExtensionsToolArguments args) throws Exception;
I expect at least:

1. get usage

2. parse arguments

3. run

probably in future run should be split as well
Line 6:


http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java:

Line 2:
Line 3:
Line 4: import org.ovirt.engine.exttool.core.ModuleService;
Line 5:
Line 6: public interface LoggerService extends ModuleService {
so why do you need this interface?
Line 7:


http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/resources/META-INF/services/org.ovirt.engine.exttool.logger.services.LoggerService
File backend/manager/extension-tool/src/main/resources/META-INF/services/org.ovirt.engine.exttool.logger.services.LoggerService:

Line 1: org.ovirt.engine.exttool.logger.services.LoggerServiceImpl
please make sure you put new lines at end of file


http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/help.properties
File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/help.properties:

Line 8: \n\t\t\t\tLogger::FLUSH\
Line 9: \n\t\t\t\tLogger::CLOSE\
Line 10: \n\t\t\t--logger-name="org.ovirt.engine"\
Line 11: \n\t\t\t--level=INFO\
Line 12: \n\t\t\t--message="test"
hmmm... won't it be simpler to just put text file as resource?


http://gerrit.ovirt.org/#/c/37886/1/ovirt-engine.spec.in
File ovirt-engine.spec.in:

Line 655
Line 656
Line 657
Line 658
Line 659
please sort()


Line 1127: %{engine_data}/bin/engine-config.sh
Line 1128: %{engine_data}/bin/engine-manage-domains.sh
Line 1129: %{engine_data}/bin/engine-prolog.sh
Line 1130: %{engine_data}/bin/ovirt-engine-role.sh
Line 1131: %{engine_data}/bin/ovirt-engine-extensions-tool.sh
please sort()
Line 1132: %{engine_data}/conf/jaas.conf
Line 1133: %{engine_data}/conf/notifier-logging.properties
Line 1134: %{engine_data}/conf/tools-logging.properties
Line 1135: %{engine_data}/services/ovirt-engine-notifier


http://gerrit.ovirt.org/#/c/37886/1/packaging/bin/ovirt-engine-extensions-tool.sh
File packaging/bin/ovirt-engine-extensions-tool.sh:

Line 25: -jar "${JBOSS_HOME}/jboss-modules.jar" \
Line 26: -dependencies org.ovirt.engine.core.extension-tool \
Line 27: -cp "${CLASSPATH}:${ENGINE_USR}/conf/extensions-tool/commons-logging.properties" \
Line 28: -class org.ovirt.engine.exttool.core.ExtensionsToolExecutor \
Line 29: "$@"
please copy as much as you can packaging/bin/engine-manage-domains.sh, for example no need for classpath and such


http://gerrit.ovirt.org/#/c/37886/1/pom.xml
File pom.xml:

Line 45: <!-- Dependencies Versions -->
Line 46: <junit.version>4.11</junit.version>
Line 47: <commons-codec.version>1.4</commons-codec.version>
Line 48: <commons-lang.version>2.6</commons-lang.version>
Line 49: <commons-logging.version>1.2</commons-logging.version>
please use only slf4j
Line 50: <commons-compress.version>1.4.1</commons-compress.version>
Line 51: <quartz.version>2.1.2</quartz.version>
Line 52: <postgres.jdbc.version>9.1-901-1.jdbc4</postgres.jdbc.version>
Line 53: <commons-collections>3.2.1</commons-collections>
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-HasComments: Yes
oVirt Jenkins CI Server
2015-02-17 20:18:52 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 1: Code-Review-1 Verified-1

Build Unstable

http://jenkins.ovirt.org/job/ovirt-engine_master_create-rpms-quick_gerrit/6172/ : The patch does not pass the tests

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/27160/ : NOT_BUILT

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/26674/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

http://jenkins.ovirt.org/job/ovirt_engine_master_compile_checkstyle_gerrit/43361/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
m***@gmail.com
2015-02-17 20:37:41 UTC
Permalink
Ondřej Macháček has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolArguments.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolArguments.java:

Line 6: import java.util.Map;
Line 7: import java.util.Properties;
Line 8: import java.util.Set;
Line 9:
Line 10: import com.sun.org.apache.xpath.internal.operations.Mod;
Post by Alon Bar-Lev
what is that?
Hmm, an accident, will remove.
Line 11: import org.ovirt.engine.core.uutils.cli.ArgumentBuilder;
Line 12: import org.ovirt.engine.core.uutils.cli.ExtendedCliParser;
Line 13:
Line 14: public class ExtensionsToolArguments {


http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java:

Line 2:
Line 3:
Line 4: import org.ovirt.engine.exttool.core.ModuleService;
Line 5:
Line 6: public interface LoggerService extends ModuleService {
Post by Alon Bar-Lev
so why do you need this interface?
That's how jdk ServiceLoader works.
I need to specify interface name, and I can't two or more implementations, for
one interface name, I will have to specify such interfaces for every module.
But I will access to them via ModulerService.

this is only because of the serviceLoader.
Line 7:


http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/help.properties
File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/help.properties:

Line 8: \n\t\t\t\tLogger::FLUSH\
Line 9: \n\t\t\t\tLogger::CLOSE\
Line 10: \n\t\t\t--logger-name="org.ovirt.engine"\
Line 11: \n\t\t\t--level=INFO\
Line 12: \n\t\t\t--message="test"
Post by Alon Bar-Lev
hmmm... won't it be simpler to just put text file as resource?
yes, will do


http://gerrit.ovirt.org/#/c/37886/1/packaging/bin/ovirt-engine-extensions-tool.sh
File packaging/bin/ovirt-engine-extensions-tool.sh:

Line 25: -jar "${JBOSS_HOME}/jboss-modules.jar" \
Line 26: -dependencies org.ovirt.engine.core.extension-tool \
Line 27: -cp "${CLASSPATH}:${ENGINE_USR}/conf/extensions-tool/commons-logging.properties" \
Line 28: -class org.ovirt.engine.exttool.core.ExtensionsToolExecutor \
Post by Alon Bar-Lev
please copy as much as you can packaging/bin/engine-manage-domains.sh, for
ok
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-02-17 20:41:57 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java:

Line 2:
Line 3:
Line 4: import org.ovirt.engine.exttool.core.ModuleService;
Line 5:
Line 6: public interface LoggerService extends ModuleService {
Post by m***@gmail.com
That's how jdk ServiceLoader works.
yes, your interface is ModuleService, the core should not care what we test... it only cares about what module is available, delegate usage, parse parameters and run.
Line 7:
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
m***@gmail.com
2015-02-17 20:52:12 UTC
Permalink
Ondřej Macháček has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java:

Line 2:
Line 3:
Line 4: import org.ovirt.engine.exttool.core.ModuleService;
Line 5:
Line 6: public interface LoggerService extends ModuleService {
Post by Alon Bar-Lev
yes, your interface is ModuleService, the core should not care what we test
Then I can't use jdk ServiceLoader, but find different way, which suite.
ServiceLoader can't have specified multiple implementaion for one interface.
Line 7:
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-02-17 20:58:45 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java:

Line 2:
Line 3:
Line 4: import org.ovirt.engine.exttool.core.ModuleService;
Line 5:
Line 6: public interface LoggerService extends ModuleService {
Post by m***@gmail.com
Then I can't use jdk ServiceLoader, but find different way, which suite.
hmmm.... you find all factories that implement an interface, this is exactly the point of service loader.

the service name is the interface name, and within you can have multiple lines, each is a class name that implements that service.
Line 7:
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
m***@gmail.com
2015-02-17 21:03:16 UTC
Permalink
Ondřej Macháček has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerService.java:

Line 2:
Line 3:
Line 4: import org.ovirt.engine.exttool.core.ModuleService;
Line 5:
Line 6: public interface LoggerService extends ModuleService {
Post by Alon Bar-Lev
hmmm.... you find all factories that implement an interface, this is exactl
oh, ok. I got it, thanks.
Line 7:
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
m***@gmail.com
2015-02-18 12:07:50 UTC
Permalink
Ondřej Macháček has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/pom.xml
File backend/manager/extension-tool/pom.xml:

Line 39: <groupId>commons-logging</groupId>
Line 40: <artifactId>commons-logging</artifactId>
Line 41: <version>${commons-logging.version}</version>
Line 42: <scope>compile</scope>
Line 43: </dependency>
Post by Alon Bar-Lev
log4j please
Done
Line 44: </dependencies>
Line 45:
Line 46: <build>
Line 47: <plugins>


http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/Module.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/Module.java:

Line 1: package org.ovirt.engine.exttool.core;
Line 2:
Line 3: public enum Module {
Post by Alon Bar-Lev
this class is bad as you do not want to patch it when adding new modules, t
removed
Line 4:
Line 5: AAA("aaa"),
Line 6: LOGGER("logger");
Line 7:


http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java:

Line 1: package org.ovirt.engine.exttool.core;
Line 2:
Line 3: public interface ModuleService {
Line 4:
Line 5: public void runModule(ExtensionsToolArguments args) throws Exception;
hmm, shouldn't be parsing args global? not per service?
Line 6:


http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/resources/META-INF/services/org.ovirt.engine.exttool.logger.services.LoggerService
File backend/manager/extension-tool/src/main/resources/META-INF/services/org.ovirt.engine.exttool.logger.services.LoggerService:

Line 1: org.ovirt.engine.exttool.logger.services.LoggerServiceImpl
Post by Alon Bar-Lev
please make sure you put new lines at end of file
Done


http://gerrit.ovirt.org/#/c/37886/1/ovirt-engine.spec.in
File ovirt-engine.spec.in:

Line 655
Line 656
Line 657
Line 658
Line 659
Post by Alon Bar-Lev
please sort()
Done


Line 1127: %{engine_data}/bin/engine-config.sh
Line 1128: %{engine_data}/bin/engine-manage-domains.sh
Line 1129: %{engine_data}/bin/engine-prolog.sh
Line 1130: %{engine_data}/bin/ovirt-engine-role.sh
Line 1131: %{engine_data}/bin/ovirt-engine-extensions-tool.sh
Post by Alon Bar-Lev
please sort()
Done
Line 1132: %{engine_data}/conf/jaas.conf
Line 1133: %{engine_data}/conf/notifier-logging.properties
Line 1134: %{engine_data}/conf/tools-logging.properties
Line 1135: %{engine_data}/services/ovirt-engine-notifier
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
Alon Bar-Lev
2015-02-18 12:12:29 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/37886/1/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java:

Line 1: package org.ovirt.engine.exttool.core;
Line 2:
Line 3: public interface ModuleService {
Line 4:
Line 5: public void runModule(ExtensionsToolArguments args) throws Exception;
Post by m***@gmail.com
hmm, shouldn't be parsing args global? not per service?
well, not sure what "parsing" is, but every service knows his own parameters... so infra must consult the module.

the simplest:

xxx --help
<get a list of modules>

xxx module --help
<get a list of options for that module>

notice the core does not care what modules are in there, all is dynamic acquired from service.
Line 6:
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
a***@ovirt.org
2015-02-18 15:41:09 UTC
Permalink
***@ovirt.org has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
m***@gmail.com
2015-02-18 15:42:41 UTC
Permalink
Ondřej Macháček has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 2:

This is final skeleton, please comment. Then I will improve
command line arg parsing.
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-02-18 16:23:30 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 2: Code-Review-1 Verified-1

Build Unstable

http://jenkins.ovirt.org/job/ovirt-engine_master_create-rpms-quick_gerrit/6187/ : The patch does not pass the tests

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/27242/ : NOT_BUILT

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/26757/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

http://jenkins.ovirt.org/job/ovirt_engine_master_compile_checkstyle_gerrit/43443/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Alon Bar-Lev
2015-02-18 22:57:36 UTC
Permalink
Alon Bar-Lev has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 2:

(23 comments)

http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolExecutor.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolExecutor.java:

Line 4: import java.util.Map;
Line 5:
Line 6: public class ExtensionsToolExecutor {
Line 7:
Line 8: public static void main(String[] args) {
in main you should setup log level and file [at least] before.
Line 9: Map<String, String> toolArguments = null;
Line 10: StringBuilder sb = new StringBuilder();
Line 11: try {
Line 12: List<ModuleService> moduleServices = ExtensionsToolLoader.load(ModuleService.class);


Line 8: public static void main(String[] args) {
Line 9: Map<String, String> toolArguments = null;
Line 10: StringBuilder sb = new StringBuilder();
Line 11: try {
Line 12: List<ModuleService> moduleServices = ExtensionsToolLoader.load(ModuleService.class);
please load all modules and put them nicely into a map.

then you can process this map instead of ugly code.
Line 13: for(ModuleService moduleService : moduleServices) {
Line 14: sb.append(moduleService.getName() + " ");
Line 15: if ((args.length < 1) || !moduleService.getName().equals(args[0])) {
Line 16: continue;


Line 14: sb.append(moduleService.getName() + " ");
Line 15: if ((args.length < 1) || !moduleService.getName().equals(args[0])) {
Line 16: continue;
Line 17: }
Line 18: toolArguments = moduleService.parseArguments(args);
if all ok, then we need to load extensions here... the extensions directory or module are global parameters.
Line 19: moduleService.run(toolArguments);
Line 20: }
Line 21:
Line 22: if(toolArguments == null) {


http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolLoader.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolLoader.java:

Line 14: services.add(module);
Line 15: }
Line 16: }
Line 17: return services;
Line 18: }
this can be simple method within the previous class, unless you think it should be reused.


http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java:

Line 2:
Line 3: import java.io.*;
Line 4: import java.util.Map;
Line 5:
Line 6: public abstract class ModuleService {
please use only interfaces for stuff we provide within service. it should be pure abstract.
Line 7:
Line 8: public static final String ARG_LOG_LEVEL = "--log-level";
Line 9: public static final String ARG_LOG_FILE = "--log-file";
Line 10: public static final String ARG_EXT_CONFIG = "--extensions-config";


Line 6: public abstract class ModuleService {
Line 7:
Line 8: public static final String ARG_LOG_LEVEL = "--log-level";
Line 9: public static final String ARG_LOG_FILE = "--log-file";
Line 10: public static final String ARG_EXT_CONFIG = "--extensions-config";
these are not per module but per core.
Line 11: private static final String HELP = "help";
Line 12:
Line 13: public String getUsage(Class module) throws Exception {
Line 14: BufferedReader reader = new BufferedReader(new InputStreamReader(module.getResourceAsStream(HELP)));


Line 7:
Line 8: public static final String ARG_LOG_LEVEL = "--log-level";
Line 9: public static final String ARG_LOG_FILE = "--log-file";
Line 10: public static final String ARG_EXT_CONFIG = "--extensions-config";
Line 11: private static final String HELP = "help";
I do not think help should be valid.
Line 12:
Line 13: public String getUsage(Class module) throws Exception {
Line 14: BufferedReader reader = new BufferedReader(new InputStreamReader(module.getResourceAsStream(HELP)));
Line 15: StringBuilder out = new StringBuilder();


Line 18: out.append(line);
Line 19: out.append(System.getProperty("line.separator"));
Line 20: }
Line 21: reader.close();
Line 22: return out.toString();
you can just read entire content and convert to UTF-8, no reason to do the line magic. worse case you replace all \n with line.separator before print to console, but I do not think it is required, as java System.print("XXX\n") works on all platforms.
Line 23: }
Line 24:
Line 25: public void printHelp(Class module) {
Line 26: try {


Line 19: out.append(System.getProperty("line.separator"));
Line 20: }
Line 21: reader.close();
Line 22: return out.toString();
Line 23: }
this should be in utility class not in base
Line 24:
Line 25: public void printHelp(Class module) {
Line 26: try {
Line 27: System.out.println(getUsage(module));


Line 28: } catch (Exception ex) {
Line 29: ex.printStackTrace();
Line 30: System.err.println("Error reading help content.");
Line 31: }
Line 32: }
not sure what this is.
Line 33:
Line 34: public abstract Map<String, String> parseArguments(String[] args) throws Exception;
Line 35:
Line 36: public abstract void run(Map<String, String> args) throws Exception;


Line 34: public abstract Map<String, String> parseArguments(String[] args) throws Exception;
Line 35:
Line 36: public abstract void run(Map<String, String> args) throws Exception;
Line 37:
Line 38: public abstract String getName();
these I understand :)
Line 39:


http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerServiceImpl.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerServiceImpl.java:

Line 24: public static final String LOG_RECORD = "log-record";
Line 25: public static final String ARG_ACTION_NAME = "--logger-name";
Line 26: public static final String ARG_ACTION_LEVEL = "--level";
Line 27: public static final String ARG_ACTION_MSG = "--message";
Line 28: public static final String ACTION_HELP = "--help";
one time used strings can be just specified, there is no need for constants.
Line 29:
Line 30: public LoggerServiceImpl() {
Line 31: }
Line 32:


Line 47: if(argMap.containsKey(ACTION_HELP)) {
Line 48: action = ACTION_HELP;
Line 49: }
Line 50:
Line 51: return argMap;
you can store this as member, no reason to return.
Line 52: }
Line 53:
Line 54: @Override
Line 55: public void run(Map<String, String> args) throws Exception {


Line 60:
Line 61: File loggerConfig = new File(args.<String> get(ARG_EXT_CONFIG));
Line 62: if (!loggerConfig.exists()) {
Line 63: throw new Exception("Could not find logger extension configuration at the specified path");
Line 64: }
this should be loaded by core and be ready.

you should have already constructed and initialized extensionmanager instance passed into the service.
Line 65: ExtensionsManager extensionsManager = new ExtensionsManager();
Line 66: ExtensionProxy extensionProxy = extensionsManager.initialize(extensionsManager.load(loggerConfig));
Line 67: LogRecord logRecord = new LogRecord(
Line 68: Level.parse(args.get(ARG_ACTION_LEVEL)),


Line 63: throw new Exception("Could not find logger extension configuration at the specified path");
Line 64: }
Line 65: ExtensionsManager extensionsManager = new ExtensionsManager();
Line 66: ExtensionProxy extensionProxy = extensionsManager.initialize(extensionsManager.load(loggerConfig));
Line 67: LogRecord logRecord = new LogRecord(
I think we should also add log name parameter, maybe others.
Line 68: Level.parse(args.get(ARG_ACTION_LEVEL)),
Line 69: args.get(ARG_ACTION_MSG)
Line 70: );
Line 71: ExtMap output = extensionProxy.invoke(


Line 75: ).mput(
Line 76: Logger.InvokeKeys.LOG_RECORD,
Line 77: logRecord
Line 78: )
Line 79: );
please flush and close as well, oh! you are doing something strange....

the sequence should be static....

publish
flush
close

so per single invocation we do all.
Line 80: }
Line 81:
Line 82: private ExtendedCliParser initParser(String action) {
Line 83: ExtendedCliParser parser = new ExtendedCliParser();


Line 122: }
Line 123:
Line 124: @Override
Line 125: public String getName() {
Line 126: return MODULE;
not sure why this constant is good... :)

just put the string.
Line 127: }
Line 128:
Line 129: private ExtUUID getCommand(String command) {
Line 130: if(command == null) {


Line 123:
Line 124: @Override
Line 125: public String getName() {
Line 126: return MODULE;
Line 127: }
please put trivials like these first in interface.
Line 128:
Line 129: private ExtUUID getCommand(String command) {
Line 130: if(command == null) {
Line 131: return Logger.InvokeCommands.PUBLISH;


http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/help
File backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/help:

put .txt suffix so we know what it is when running zipinfo
Line 1: ovirt-engine-extension-tool
Line 2: --log-level=INFO
Line 3: --log-file=
Line 4: --extensions-config=<location of extensions configuration>


Line 8: FLUSH
Line 9: CLOSE
Line 10: --logger-name="org.ovirt.engine"
Line 11: --level=INFO
Line 12: --message="test"
please always put new lines in sources


http://gerrit.ovirt.org/#/c/37886/2/ovirt-engine.spec.in
File ovirt-engine.spec.in:

Line 660: %{engine_jboss_modules}/common/org/ovirt/engine/core/tools/main/tools.jar
Line 661: %{engine_jboss_modules}/common/org/ovirt/engine/core/utils/main/utils.jar
Line 662: %{engine_jboss_modules}/common/org/ovirt/engine/core/uutils/main/uutils.jar
Line 663: %{engine_jboss_modules}/common/org/ovirt/engine/extensions/builtin/main/builtin.jar
Line 664: %{engine_jboss_modules}/org/ovirt/engine/core/extension-tool/main/extension-tool.jar
hmmm.... it should be in common I think.
Line 665: __EOF__
Line 666:
Line 667: # Needed for compatibility if package is different than the directory structure
Line 668: %if "%{name}" != "%{engine_name}"


http://gerrit.ovirt.org/#/c/37886/2/packaging/bin/ovirt-engine-extensions-tool.sh
File packaging/bin/ovirt-engine-extensions-tool.sh:

Line 20:
Line 21: exec "${JAVA_HOME}/bin/java" \
Line 22: -Djboss.modules.write-indexes=false \
Line 23: -Djava.security.auth.login.config="${ENGINE_USR}/conf/jaas.conf" \
Line 24: -Djava.util.logging.config.file="${ENGINE_USR}/conf/extensions-tool/logging.conf" \
-Djava.util.logging.config.file="${OVIRT_LOGGING_PROPERTIES}" \
Line 25: -jar "${JBOSS_HOME}/jboss-modules.jar" \
Line 26: -dependencies org.ovirt.engine.core.extension-tool \
Line 27: -cp "${CLASSPATH}:${ENGINE_USR}/conf/extensions-tool/commons-logging.properties" \
Line 28: -class org.ovirt.engine.exttool.core.ExtensionsToolExecutor \


Line 23: -Djava.security.auth.login.config="${ENGINE_USR}/conf/jaas.conf" \
Line 24: -Djava.util.logging.config.file="${ENGINE_USR}/conf/extensions-tool/logging.conf" \
Line 25: -jar "${JBOSS_HOME}/jboss-modules.jar" \
Line 26: -dependencies org.ovirt.engine.core.extension-tool \
Line 27: -cp "${CLASSPATH}:${ENGINE_USR}/conf/extensions-tool/commons-logging.properties" \
why do we need commons-logging?
Line 28: -class org.ovirt.engine.exttool.core.ExtensionsToolExecutor \
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
a***@ovirt.org
2015-02-23 20:48:55 UTC
Permalink
***@ovirt.org has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
m***@gmail.com
2015-02-23 20:49:14 UTC
Permalink
Ondřej Macháček has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 2:

(21 comments)

http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolExecutor.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolExecutor.java:

Line 8: public static void main(String[] args) {
Line 9: Map<String, String> toolArguments = null;
Line 10: StringBuilder sb = new StringBuilder();
Line 11: try {
Line 12: List<ModuleService> moduleServices = ExtensionsToolLoader.load(ModuleService.class);
Post by Alon Bar-Lev
please load all modules and put them nicely into a map.
Done
Line 13: for(ModuleService moduleService : moduleServices) {
Line 14: sb.append(moduleService.getName() + " ");
Line 15: if ((args.length < 1) || !moduleService.getName().equals(args[0])) {
Line 16: continue;


Line 14: sb.append(moduleService.getName() + " ");
Line 15: if ((args.length < 1) || !moduleService.getName().equals(args[0])) {
Line 16: continue;
Line 17: }
Line 18: toolArguments = moduleService.parseArguments(args);
Post by Alon Bar-Lev
if all ok, then we need to load extensions here... the extensions directory
Done
Line 19: moduleService.run(toolArguments);
Line 20: }
Line 21:
Line 22: if(toolArguments == null) {


http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolLoader.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ExtensionsToolLoader.java:

Line 14: services.add(module);
Line 15: }
Line 16: }
Line 17: return services;
Line 18: }
Post by Alon Bar-Lev
this can be simple method within the previous class, unless you think it sh
I think it would not, moving to method.


http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/core/ModuleService.java:

Line 2:
Line 3: import java.io.*;
Line 4: import java.util.Map;
Line 5:
Line 6: public abstract class ModuleService {
Post by Alon Bar-Lev
please use only interfaces for stuff we provide within service. it should b
Done
Line 7:
Line 8: public static final String ARG_LOG_LEVEL = "--log-level";
Line 9: public static final String ARG_LOG_FILE = "--log-file";
Line 10: public static final String ARG_EXT_CONFIG = "--extensions-config";


Line 6: public abstract class ModuleService {
Line 7:
Line 8: public static final String ARG_LOG_LEVEL = "--log-level";
Line 9: public static final String ARG_LOG_FILE = "--log-file";
Line 10: public static final String ARG_EXT_CONFIG = "--extensions-config";
Post by Alon Bar-Lev
these are not per module but per core.
Done
Line 11: private static final String HELP = "help";
Line 12:
Line 13: public String getUsage(Class module) throws Exception {
Line 14: BufferedReader reader = new BufferedReader(new InputStreamReader(module.getResourceAsStream(HELP)));


Line 7:
Line 8: public static final String ARG_LOG_LEVEL = "--log-level";
Line 9: public static final String ARG_LOG_FILE = "--log-file";
Line 10: public static final String ARG_EXT_CONFIG = "--extensions-config";
Line 11: private static final String HELP = "help";
Post by Alon Bar-Lev
I do not think help should be valid.
Done
Line 12:
Line 13: public String getUsage(Class module) throws Exception {
Line 14: BufferedReader reader = new BufferedReader(new InputStreamReader(module.getResourceAsStream(HELP)));
Line 15: StringBuilder out = new StringBuilder();


Line 18: out.append(line);
Line 19: out.append(System.getProperty("line.separator"));
Line 20: }
Line 21: reader.close();
Line 22: return out.toString();
Post by Alon Bar-Lev
you can just read entire content and convert to UTF-8, no reason to do the
Done
Line 23: }
Line 24:
Line 25: public void printHelp(Class module) {
Line 26: try {


Line 19: out.append(System.getProperty("line.separator"));
Line 20: }
Line 21: reader.close();
Line 22: return out.toString();
Line 23: }
Post by Alon Bar-Lev
this should be in utility class not in base
Done
Line 24:
Line 25: public void printHelp(Class module) {
Line 26: try {
Line 27: System.out.println(getUsage(module));


Line 28: } catch (Exception ex) {
Line 29: ex.printStackTrace();
Line 30: System.err.println("Error reading help content.");
Line 31: }
Line 32: }
Post by Alon Bar-Lev
not sure what this is.
removed
Line 33:
Line 34: public abstract Map<String, String> parseArguments(String[] args) throws Exception;
Line 35:
Line 36: public abstract void run(Map<String, String> args) throws Exception;


http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerServiceImpl.java
File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/logger/services/LoggerServiceImpl.java:

Line 24: public static final String LOG_RECORD = "log-record";
Line 25: public static final String ARG_ACTION_NAME = "--logger-name";
Line 26: public static final String ARG_ACTION_LEVEL = "--level";
Line 27: public static final String ARG_ACTION_MSG = "--message";
Line 28: public static final String ACTION_HELP = "--help";
Post by Alon Bar-Lev
one time used strings can be just specified, there is no need for constants
Done
Line 29:
Line 30: public LoggerServiceImpl() {
Line 31: }
Line 32:


Line 47: if(argMap.containsKey(ACTION_HELP)) {
Line 48: action = ACTION_HELP;
Line 49: }
Line 50:
Line 51: return argMap;
Post by Alon Bar-Lev
you can store this as member, no reason to return.
Done
Line 52: }
Line 53:
Line 54: @Override
Line 55: public void run(Map<String, String> args) throws Exception {


Line 60:
Line 61: File loggerConfig = new File(args.<String> get(ARG_EXT_CONFIG));
Line 62: if (!loggerConfig.exists()) {
Line 63: throw new Exception("Could not find logger extension configuration at the specified path");
Line 64: }
Post by Alon Bar-Lev
this should be loaded by core and be ready.
Done
Line 65: ExtensionsManager extensionsManager = new ExtensionsManager();
Line 66: ExtensionProxy extensionProxy = extensionsManager.initialize(extensionsManager.load(loggerConfig));
Line 67: LogRecord logRecord = new LogRecord(
Line 68: Level.parse(args.get(ARG_ACTION_LEVEL)),


Line 63: throw new Exception("Could not find logger extension configuration at the specified path");
Line 64: }
Line 65: ExtensionsManager extensionsManager = new ExtensionsManager();
Line 66: ExtensionProxy extensionProxy = extensionsManager.initialize(extensionsManager.load(loggerConfig));
Line 67: LogRecord logRecord = new LogRecord(
Post by Alon Bar-Lev
I think we should also add log name parameter, maybe others.
Done
Line 68: Level.parse(args.get(ARG_ACTION_LEVEL)),
Line 69: args.get(ARG_ACTION_MSG)
Line 70: );
Line 71: ExtMap output = extensionProxy.invoke(


Line 75: ).mput(
Line 76: Logger.InvokeKeys.LOG_RECORD,
Line 77: logRecord
Line 78: )
Line 79: );
Post by Alon Bar-Lev
please flush and close as well, oh! you are doing something strange....
Done
Line 80: }
Line 81:
Line 82: private ExtendedCliParser initParser(String action) {
Line 83: ExtendedCliParser parser = new ExtendedCliParser();


Line 122: }
Line 123:
Line 124: @Override
Line 125: public String getName() {
Line 126: return MODULE;
Post by Alon Bar-Lev
not sure why this constant is good... :)
habit from school
Line 127: }
Line 128:
Line 129: private ExtUUID getCommand(String command) {
Line 130: if(command == null) {


Line 123:
Line 124: @Override
Line 125: public String getName() {
Line 126: return MODULE;
Line 127: }
Post by Alon Bar-Lev
please put trivials like these first in interface.
Done
Line 128:
Line 129: private ExtUUID getCommand(String command) {
Line 130: if(command == null) {
Line 131: return Logger.InvokeCommands.PUBLISH;


http://gerrit.ovirt.org/#/c/37886/2/backend/manager/extension-tool/src/main/resources/org/ovirt/engine/exttool/logger/services/help
Post by Alon Bar-Lev
put .txt suffix so we know what it is when running zipinfo
Done
Line 1: ovirt-engine-extension-tool
Line 2: --log-level=INFO
Line 3: --log-file=
Line 4: --extensions-config=<location of extensions configuration>


Line 8: FLUSH
Line 9: CLOSE
Line 10: --logger-name="org.ovirt.engine"
Line 11: --level=INFO
Line 12: --message="test"
Post by Alon Bar-Lev
please always put new lines in sources
Done


http://gerrit.ovirt.org/#/c/37886/2/ovirt-engine.spec.in
File ovirt-engine.spec.in:

Line 660: %{engine_jboss_modules}/common/org/ovirt/engine/core/tools/main/tools.jar
Line 661: %{engine_jboss_modules}/common/org/ovirt/engine/core/utils/main/utils.jar
Line 662: %{engine_jboss_modules}/common/org/ovirt/engine/core/uutils/main/uutils.jar
Line 663: %{engine_jboss_modules}/common/org/ovirt/engine/extensions/builtin/main/builtin.jar
Line 664: %{engine_jboss_modules}/org/ovirt/engine/core/extension-tool/main/extension-tool.jar
Post by Alon Bar-Lev
hmmm.... it should be in common I think.
Done
Line 665: __EOF__
Line 666:
Line 667: # Needed for compatibility if package is different than the directory structure
Line 668: %if "%{name}" != "%{engine_name}"


http://gerrit.ovirt.org/#/c/37886/2/packaging/bin/ovirt-engine-extensions-tool.sh
File packaging/bin/ovirt-engine-extensions-tool.sh:

Line 20:
Line 21: exec "${JAVA_HOME}/bin/java" \
Line 22: -Djboss.modules.write-indexes=false \
Line 23: -Djava.security.auth.login.config="${ENGINE_USR}/conf/jaas.conf" \
Line 24: -Djava.util.logging.config.file="${ENGINE_USR}/conf/extensions-tool/logging.conf" \
Post by Alon Bar-Lev
-Djava.util.logging.config.file="${OVIRT_LOGGING_PROPERTIES}" \
Done
Line 25: -jar "${JBOSS_HOME}/jboss-modules.jar" \
Line 26: -dependencies org.ovirt.engine.core.extension-tool \
Line 27: -cp "${CLASSPATH}:${ENGINE_USR}/conf/extensions-tool/commons-logging.properties" \
Line 28: -class org.ovirt.engine.exttool.core.ExtensionsToolExecutor \


Line 23: -Djava.security.auth.login.config="${ENGINE_USR}/conf/jaas.conf" \
Line 24: -Djava.util.logging.config.file="${ENGINE_USR}/conf/extensions-tool/logging.conf" \
Line 25: -jar "${JBOSS_HOME}/jboss-modules.jar" \
Line 26: -dependencies org.ovirt.engine.core.extension-tool \
Line 27: -cp "${CLASSPATH}:${ENGINE_USR}/conf/extensions-tool/commons-logging.properties" \
Post by Alon Bar-Lev
why do we need commons-logging?
removed
Line 28: -class org.ovirt.engine.exttool.core.ExtensionsToolExecutor \
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
oVirt Jenkins CI Server
2015-02-23 20:49:57 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

Change subject: aaa: extensions test tool: logger
......................................................................


Patch Set 3:

Build Failed

http://jenkins.ovirt.org/job/ovirt-engine_master_create-rpms-quick_gerrit/6234/ : There was an infra issue, please contact ***@ovirt.org

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/27609/ : FAILURE

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/27124/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

http://jenkins.ovirt.org/job/ovirt_engine_master_compile_checkstyle_gerrit/43810/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.
--
To view, visit http://gerrit.ovirt.org/37886
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
a***@ovirt.org
2015-03-03 20:50:45 UTC
Permalink
***@ovirt.org has posted comments on this change.

Change subject: extensions test tool: logger
......................................................................


Patch Set 4:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-engine-3.5', 'ovirt-engine-3.4', 'ovirt-engine-3.3', 'ovirt-engine-3.4.0', 'ovirt-engine-3.3.4', 'ovirt-engine-3.3.3', 'ovirt-engine-3.3.2', 'ovirt-engine-3.3.1'])
--
To view, visit https://gerrit.ovirt.org/37886
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
m***@gmail.com
2015-03-03 20:52:54 UTC
Permalink
Ondřej Macháček has posted comments on this change.

Change subject: extensions test tool: logger
......................................................................


Patch Set 4:

Logger works fine, with parsing, but still could be some
small bugs. I will test more, when you do review and do last
structural changes you recommend.
--
To view, visit https://gerrit.ovirt.org/37886
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
oVirt Jenkins CI Server
2015-03-03 21:09:03 UTC
Permalink
oVirt Jenkins CI Server has posted comments on this change.

Change subject: extensions test tool: logger
......................................................................


Patch Set 4: Code-Review-1 Verified-1

Build Unstable

http://jenkins.ovirt.org/job/ovirt-engine_master_create-rpms-quick_gerrit/6355/ : The patch does not pass the tests

http://jenkins.ovirt.org/job/ovirt-engine_master_unit-tests_gerrit/28042/ : NOT_BUILT

http://jenkins.ovirt.org/job/ovirt-engine_master_find-bugs_gerrit/27559/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.

http://jenkins.ovirt.org/job/ovirt_engine_master_compile_checkstyle_gerrit/44243/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org.
--
To view, visit https://gerrit.ovirt.org/37886
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie06113c5d56a49e58d557c851f9ff00b9a9ca409
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <***@redhat.com>
Gerrit-Reviewer: Ondřej Macháček <***@gmail.com>
Gerrit-Reviewer: ***@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
Continue reading on narkive:
Loading...