nfsantos commented on code in PR #1182:
URL: https://github.com/apache/jackrabbit-oak/pull/1182#discussion_r1394017086
##########
oak-run/src/test/java/org/apache/jackrabbit/oak/index/DocumentStoreIndexerIT.java:
##########
@@ -380,6 +387,95 @@ protected CompositeIndexer prepareIndexers(NodeStore
nodeStore, NodeBuilder buil
}
+ @Test
+ public void metrics() throws Exception {
+ MongoConnection mongoConnection = getConnection();
+ DocumentNodeStoreBuilder<?> docBuilder = builderProvider.newBuilder()
+ .setMongoDB(mongoConnection.getMongoClient(),
mongoConnection.getDBName());
+ DocumentNodeStore store = docBuilder.build();
+
+ Whiteboard wb = new DefaultWhiteboard();
+ MongoDocumentStore ds = (MongoDocumentStore)
docBuilder.getDocumentStore();
+ Registration r1 = wb.register(MongoDocumentStore.class, ds,
emptyMap());
+
+ ScheduledExecutorService executor =
Executors.newSingleThreadScheduledExecutor();
+ MetricStatisticsProvider metricsStatisticsProvider = new
MetricStatisticsProvider(null, executor);
+ wb.register(StatisticsProvider.class, metricsStatisticsProvider,
emptyMap());
+ Registration c1Registration = wb.register(MongoDatabase.class,
mongoConnection.getDatabase(), emptyMap());
+
+ configureIndex(store);
+
+ NodeBuilder builder = store.getRoot().builder();
+ NodeBuilder appNB = newNode("app:Asset");
+ createChild(appNB,
+ "jcr:content",
+ "jcr:content/comments",
+ "jcr:content/metadata",
+ "jcr:content/metadata/xmp",
+ "jcr:content/renditions",
+ "jcr:content/renditions/original",
+ "jcr:content/renditions/original/jcr:content"
+ );
+ builder.child("test").setChildNode("book.jpg", appNB.getNodeState());
+ store.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY);
+
+ String checkpoint = store.checkpoint(100000);
+
+ //Shut down this store and restart in readOnly mode
+ store.dispose();
Review Comment:
This variable is used to reference two different objects. And I agree that
the test could be improved, but I just copy'n'pasted the existing test and do
not want to spend much more time in refactoring it.
##########
oak-run/src/test/java/org/apache/jackrabbit/oak/index/DocumentStoreIndexerIT.java:
##########
@@ -380,6 +387,95 @@ protected CompositeIndexer prepareIndexers(NodeStore
nodeStore, NodeBuilder buil
}
+ @Test
+ public void metrics() throws Exception {
Review Comment:
It's not so simple to share the common code because it is creating several
objects that we need during the test. In this case, I think it is not worth the
effort to improve the code, some duplication is not a big issue here as this is
testing code and the duplication is contained in the same file.
##########
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/PipelinedMergeSortTaskTest.java:
##########
@@ -89,27 +94,41 @@ private PipelinedMergeSortTask.Result runTest(Compression
algorithm, Path... fil
Path sortRoot = sortFolder.getRoot().toPath();
Review Comment:
I moved the executor to the top level, to be created before all tests and
shutdown after the tests. There was a potential resource leak because I was not
calling the shutdown method. For the `MetricStatisticsProvider`. I prefer to
create it close to the code that does the testing. Almost all tests call this
`runTest` method and the tests that don't do not need needs this object. So in
this case, allocation of this helper object is closer to where they are used,
which makes the logic simpler.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/index/importer/IndexImporter.java:
##########
@@ -465,19 +475,31 @@ interface IndexImporterStepExecutor {
}
void runWithRetry(int maxRetries, IndexImportState indexImportState,
IndexImporterStepExecutor step) throws CommitFailedException, IOException {
+ String indexImportPhaseName = indexImportState == null ? "null" :
indexImportState.toString();
int count = 1;
Stopwatch start = Stopwatch.createStarted();
while (count <= maxRetries) {
- LOG.info("IndexImporterStepExecutor:{}, count:{}",
indexImportState, count);
- LOG.info("[TASK:{}:START]", indexImportState);
+ LOG.info("IndexImporterStepExecutor:{}, count:{}",
indexImportPhaseName, count);
+ LOG.info("[TASK:{}:START]", indexImportPhaseName);
try {
step.execute();
- LOG.info("[TASK:{}:END] Metrics: {}", indexImportState,
+ long durationSeconds = start.elapsed(TimeUnit.SECONDS);
+ LOG.info("[TASK:{}:END] Metrics: {}", indexImportPhaseName,
MetricsFormatter.newBuilder()
.add("duration",
FormattingUtils.formatToSeconds(start))
- .add("durationSeconds",
start.elapsed(TimeUnit.SECONDS))
+ .add("durationSeconds", durationSeconds)
.build()
);
+
+ String name = "oak_indexer_import_" +
indexImportPhaseName.toLowerCase() + "_duration_seconds";
+ CounterStats metric = statisticsProvider.getCounterStats(name,
StatsOptions.METRICS_ONLY);
+ LOG.debug("Adding metric: {} {}", name, durationSeconds);
Review Comment:
The MetricsUtils class was not in a package accessible from from
IndexImporter. I moved MetricsUtils to a common package so it can be used more
widely.
##########
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/pipelined/PipelinedMergeSortTaskTest.java:
##########
@@ -89,27 +94,41 @@ private PipelinedMergeSortTask.Result runTest(Compression
algorithm, Path... fil
Path sortRoot = sortFolder.getRoot().toPath();
// +1 for the Sentinel.
ArrayBlockingQueue<Path> sortedFilesQueue = new
ArrayBlockingQueue<>(files.length + 1);
- PipelinedMergeSortTask mergeSortTask = new
PipelinedMergeSortTask(sortRoot, pathComparator, algorithm, sortedFilesQueue);
- // Enqueue all the files that are to be merged
- for (Path file : files) {
- // The intermediate files are deleted after being merged, so we
should copy them to the temporary sort root folder
- Path workDirCopy = Files.copy(file,
sortRoot.resolve(file.getFileName()));
- sortedFilesQueue.put(workDirCopy);
+ ScheduledExecutorService executor =
Executors.newSingleThreadScheduledExecutor();
+ try (MetricStatisticsProvider metricStatisticsProvider = new
MetricStatisticsProvider(null, executor)) {
Review Comment:
This constructor is called in many tests with null, I don't want to spread
the same comment in so many places. The constructor should have a comment
explaining that null is accepted and what that means (it does not, but that's a
separate issue). And from the comments that should be in the constructor, it
would be clear why we are passing null (i.e., the test does not need an MBean
server).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
