Skip to content

Commit

Permalink
wip - refactoring to official osgi annotations
Browse files Browse the repository at this point in the history
warning: the current state may still not be semantically
         equivalent to the state before refactoring
  • Loading branch information
jsedding committed Sep 26, 2017
1 parent bf0e797 commit 9e1dfda
Show file tree
Hide file tree
Showing 49 changed files with 1,095 additions and 1,011 deletions.
25 changes: 25 additions & 0 deletions REFACOTRING-SCR-NOTES.md
@@ -0,0 +1,25 @@
Refactoring SCR Annotations
===========================

Tools
-----

This branch contains a bash script `compare.sh`, which allows comparing the declarative services and metatype xml files of two jar files.

It takes the Oak module (e.g. oak-core) and the version (e.g. 1.8-SNAPSHOT) as arguments. Optionally also a path that points to the location of an XML file inside the bundle.

./compare.sh oak-core 1.8-SNAPSHOT OSGI-INF/org.apache.jackrabbit.oak.plugins.index.datastore.DataStoreTextProviderService.xml

To better undertand what the script does you can run bash in debug mode:

bash -x ./compare.sh oak-core 1.8-SNAPSHOT

Any output of the script is written to `./comparison/` and cleared when the script is run the next time.

The baseline jar file is looked up in the local maven repository (i.e. `~/.m2/repository`).

Note
----

There are some differences in the XML files that do not represent a semantic difference. Therefore the diff still needs to be manually inspected.

3 changes: 2 additions & 1 deletion compare.sh
Expand Up @@ -56,12 +56,13 @@ for file in $CHANGED_FILES; do
for i in "${OLD_PATH}" "${NEW_PATH}"; do
sed -i'.orig' -E 's/ servicefactory="false"//g' "${i}"
sed -i'.bak' -E 's/type="String" //g' "${i}"
sed -i'.bak' -E 's/( id=".*)\$Configuration/\1/g' "${i}"
sed -i'.bak' -E 's/ xmlns:(scr|metatype)=".*"//g' "${i}" && \
sed -i'.bak' -E 's/(<\/?)(scr|metatype):/\1/g' "${i}"
echo "${i}" | grep -q 'metatype' && sed -i'.bak' -E 's/(name|description)=".*"/\1="\[\[noise\]\]"/g' "${i}"
done
mkdir -p "diffs/${FILE_DIR}"
DIFF_FILE="diffs/${FILE_DIR}/${FILE_NAME}.diff"
DIFF_FILE="diffs/${FILE_DIR}/${FILE_NAME}.diff"
python xmldiffs.py "${OLD_PATH}" "${NEW_PATH}" > "${DIFF_FILE}"

cat "${DIFF_FILE}" >> diffs.log
Expand Down
9 changes: 6 additions & 3 deletions oak-core/pom.xml
Expand Up @@ -141,9 +141,12 @@
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.apache.felix</groupId>
<artifactId>org.apache.felix.scr.annotations</artifactId>
<scope>provided</scope>
<groupId>org.osgi</groupId>
<artifactId>org.osgi.service.component.annotations</artifactId>
</dependency>
<dependency>
<groupId>org.osgi</groupId>
<artifactId>org.osgi.service.metatype.annotations</artifactId>
</dependency>

<!-- Dependencies to other Oak components -->
Expand Down
Expand Up @@ -32,9 +32,6 @@
import javax.management.openmbean.TabularDataSupport;
import javax.management.openmbean.TabularType;

import org.apache.felix.scr.annotations.Activate;
import org.apache.felix.scr.annotations.Component;
import org.apache.felix.scr.annotations.Deactivate;
import org.apache.jackrabbit.oak.api.jmx.CacheStatsMBean;
import org.apache.jackrabbit.oak.api.jmx.ConsolidatedCacheStatsMBean;
import org.apache.jackrabbit.oak.api.jmx.PersistentCacheStatsMBean;
Expand All @@ -43,12 +40,15 @@
import org.apache.jackrabbit.oak.spi.whiteboard.Tracker;
import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard;
import org.osgi.framework.BundleContext;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Deactivate;

import static org.apache.jackrabbit.oak.cache.CacheStats.timeInWords;
import static org.apache.jackrabbit.oak.commons.IOUtils.humanReadableByteCount;
import static org.apache.jackrabbit.oak.spi.whiteboard.WhiteboardUtils.registerMBean;

@Component
@Component(service = {})
public class ConsolidatedCacheStats implements ConsolidatedCacheStatsMBean {

private Tracker<CacheStatsMBean> cacheStats;
Expand Down
Expand Up @@ -16,24 +16,13 @@
*/
package org.apache.jackrabbit.oak.plugins.atomic;

import static org.apache.felix.scr.annotations.ReferenceCardinality.OPTIONAL_UNARY;
import static org.apache.felix.scr.annotations.ReferencePolicy.DYNAMIC;

import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.atomic.AtomicReference;

import javax.annotation.Nullable;

import org.apache.felix.scr.annotations.Activate;
import org.apache.felix.scr.annotations.Component;
import org.apache.felix.scr.annotations.Deactivate;
import org.apache.felix.scr.annotations.Property;
import org.apache.felix.scr.annotations.Reference;
import org.apache.felix.scr.annotations.ReferenceCardinality;
import org.apache.felix.scr.annotations.ReferencePolicy;
import org.apache.felix.scr.annotations.Service;
import org.apache.jackrabbit.oak.api.CommitFailedException;
import org.apache.jackrabbit.oak.commons.concurrent.ExecutorCloser;
import org.apache.jackrabbit.oak.osgi.OsgiWhiteboard;
Expand All @@ -47,26 +36,31 @@
import org.apache.jackrabbit.oak.spi.state.NodeStore;
import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard;
import org.osgi.framework.BundleContext;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Deactivate;
import org.osgi.service.component.annotations.Reference;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.google.common.base.Supplier;
import com.google.common.util.concurrent.ThreadFactoryBuilder;

import static org.osgi.service.component.annotations.ReferenceCardinality.OPTIONAL;
import static org.osgi.service.component.annotations.ReferencePolicy.DYNAMIC;

/**
* Provide an instance of {@link AtomicCounterEditor}. See {@link AtomicCounterEditor} for
* behavioural details.
*/
@Component
@Property(name = "type", value = "atomicCounter", propertyPrivate = true)
@Service(EditorProvider.class)
@Component(
property = "type=atomicCounter",
service = EditorProvider.class)
public class AtomicCounterEditorProvider implements EditorProvider {
private static final Logger LOG = LoggerFactory.getLogger(AtomicCounterEditorProvider.class);

@Reference(policy = ReferencePolicy.DYNAMIC, cardinality = ReferenceCardinality.OPTIONAL_UNARY, referenceInterface = Clusterable.class)
private AtomicReference<Clusterable> cluster = new AtomicReference<Clusterable>();

@Reference(policy = DYNAMIC, cardinality = OPTIONAL_UNARY, referenceInterface = NodeStore.class)
private volatile AtomicReference<NodeStore> store = new AtomicReference<NodeStore>();

private volatile AtomicReference<ScheduledExecutorService> scheduler = new AtomicReference<ScheduledExecutorService>();
Expand Down Expand Up @@ -193,6 +187,7 @@ public void deactivate() {
}
}

@Reference(name = "cluster", policy = DYNAMIC, cardinality = OPTIONAL)
protected void bindCluster(Clusterable store) {
this.cluster.set(store);
}
Expand All @@ -201,6 +196,7 @@ protected void unbindCluster(Clusterable store) {
this.cluster.compareAndSet(store, null);
}

@Reference(name = "store", policy = DYNAMIC, cardinality = OPTIONAL)
protected void bindStore(NodeStore store) {
this.store.set(store);
}
Expand Down
Expand Up @@ -16,19 +16,17 @@
*/
package org.apache.jackrabbit.oak.plugins.commit;

import org.apache.felix.scr.annotations.Component;
import org.apache.felix.scr.annotations.Service;
import org.apache.jackrabbit.oak.spi.commit.CommitInfo;
import org.apache.jackrabbit.oak.spi.commit.EditorProvider;
import org.apache.jackrabbit.oak.spi.commit.Validator;
import org.apache.jackrabbit.oak.spi.commit.ValidatorProvider;
import org.apache.jackrabbit.oak.spi.state.NodeState;
import org.osgi.service.component.annotations.Component;

/**
* TODO document
*/
@Component
@Service(EditorProvider.class)
@Component(service = EditorProvider.class)
public class ConflictValidatorProvider extends ValidatorProvider {

@Override
Expand Down
Expand Up @@ -16,14 +16,6 @@
*/
package org.apache.jackrabbit.oak.plugins.cow;

import org.apache.felix.scr.annotations.Activate;
import org.apache.felix.scr.annotations.Component;
import org.apache.felix.scr.annotations.ConfigurationPolicy;
import org.apache.felix.scr.annotations.Deactivate;
import org.apache.felix.scr.annotations.Property;
import org.apache.felix.scr.annotations.Reference;
import org.apache.felix.scr.annotations.ReferenceCardinality;
import org.apache.felix.scr.annotations.ReferencePolicy;
import org.apache.jackrabbit.oak.api.jmx.CopyOnWriteStoreMBean;
import org.apache.jackrabbit.oak.commons.PropertiesUtil;
import org.apache.jackrabbit.oak.osgi.OsgiWhiteboard;
Expand All @@ -36,6 +28,13 @@
import org.osgi.framework.Constants;
import org.osgi.framework.ServiceRegistration;
import org.osgi.service.component.ComponentContext;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.ConfigurationPolicy;
import org.osgi.service.component.annotations.Deactivate;
import org.osgi.service.component.annotations.Reference;
import org.osgi.service.component.annotations.ReferenceCardinality;
import org.osgi.service.component.annotations.ReferencePolicy;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -45,18 +44,21 @@

import static org.apache.jackrabbit.oak.spi.whiteboard.WhiteboardUtils.registerMBean;

@Component(policy = ConfigurationPolicy.REQUIRE)
@Component(
configurationPolicy = ConfigurationPolicy.REQUIRE,
service = {})
public class COWNodeStoreService {

private static final Logger LOG = LoggerFactory.getLogger(COWNodeStoreService.class);

@Property(
label = "NodeStoreProvider role",
description = "Property indicating that this component will not register as a NodeStore but as a NodeStoreProvider with given role"
)
/**
* NodeStoreProvider role
* <br>
* Property indicating that this component will not register as a
* NodeStore but as a NodeStoreProvider with given role
*/
public static final String PROP_ROLE = "role";

@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY, policy = ReferencePolicy.DYNAMIC, target = "(role=copy-on-write)", bind = "bindNodeStoreProvider", unbind = "unbindNodeStoreProvider")
private NodeStoreProvider nodeStoreProvider;

private String nodeStoreDescription;
Expand Down Expand Up @@ -162,6 +164,11 @@ private void unregisterNodeStore() {
}
}

@Reference(
name = "nodeStoreProvider",
cardinality = ReferenceCardinality.MANDATORY,
policy = ReferencePolicy.DYNAMIC,
target = "(role=copy-on-write)")
protected void bindNodeStoreProvider(NodeStoreProvider ns, Map<String, ?> config) {
this.nodeStoreProvider = ns;
this.nodeStoreDescription = PropertiesUtil.toString(config.get("oak.nodestore.description"), ns.getClass().getName());
Expand Down
Expand Up @@ -30,13 +30,6 @@
import javax.annotation.Nonnull;
import javax.jcr.Value;

import org.apache.felix.scr.annotations.Activate;
import org.apache.felix.scr.annotations.Component;
import org.apache.felix.scr.annotations.Deactivate;
import org.apache.felix.scr.annotations.Reference;
import org.apache.felix.scr.annotations.ReferenceCardinality;
import org.apache.felix.scr.annotations.ReferencePolicy;
import org.apache.felix.scr.annotations.Service;
import org.apache.jackrabbit.commons.SimpleValueFactory;
import org.apache.jackrabbit.oak.api.Descriptors;
import org.apache.jackrabbit.oak.osgi.OsgiWhiteboard;
Expand All @@ -46,6 +39,12 @@
import org.apache.jackrabbit.oak.spi.state.NodeState;
import org.osgi.framework.Version;
import org.osgi.service.component.ComponentContext;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Deactivate;
import org.osgi.service.component.annotations.Reference;
import org.osgi.service.component.annotations.ReferenceCardinality;
import org.osgi.service.component.annotations.ReferencePolicy;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -136,8 +135,10 @@
*
* @see #OAK_DISCOVERYLITE_CLUSTERVIEW
*/
@Component(immediate = true, name = DocumentDiscoveryLiteService.COMPONENT_NAME)
@Service(value = { DocumentDiscoveryLiteService.class, Observer.class })
@Component(
name = DocumentDiscoveryLiteService.COMPONENT_NAME,
immediate = true,
service = { DocumentDiscoveryLiteService.class, Observer.class })
public class DocumentDiscoveryLiteService implements ClusterStateChangeListener, Observer {

static final String COMPONENT_NAME = "org.apache.jackrabbit.oak.plugins.document.DocumentDiscoveryLiteService";
Expand Down Expand Up @@ -287,8 +288,8 @@ public Value[] getValues(String key) {
* Require a static reference to the NodeStore. Note that this implies the
* service is only active for documentNS
**/
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY, policy = ReferencePolicy.STATIC)
private volatile DocumentNodeStore nodeStore;
@Reference
private DocumentNodeStore nodeStore;

This comment has been minimized.

Copy link
@rombert

rombert Sep 26, 2017

Why is volatile no longer needed?

This comment has been minimized.

Copy link
@jsedding

jsedding Sep 26, 2017

Author Owner

I think it never was needed here. That's because the reference is static, so by the time the service gets published it is fully initialized. I.e. the field is never modified once it leaves the initializing thread. AFAIK volatile is only needed for fields with ReferencePolicy.DYNAMIC.

This comment has been minimized.

Copy link
@rombert

rombert Sep 27, 2017

Right, that makes sense.


/**
* inactive nodes that have been so for a while, ie they have no backlog
Expand Down

0 comments on commit 9e1dfda

Please sign in to comment.