Update Azure Storage SDK to v12, add logging for Azure Blob storage directory #198

nzdev · 2020-12-04T10:18:36Z

Expanding on the great work by @bielu.

What?
Refactors AzureDirectory related classes to be more open for extension and deduplicates code.
Upgrades the Azure blob SDK to v12 as v11 is obsolete. This required changing the framework version for AzureDirectory, Tests and Demo projects to .NET 4.6.1

Why?
Make the Azure Blob Storage Directory easier to troubleshoot so it can be made more reliable.

How to test?
Run the Azurite emulator or azure storage emulator
Run all the tests in the test project.

…ob-v1 # Conflicts: # build/build.xml

…ob-v1 # Conflicts: # src/Examine.AzureDirectory/AzureDirectory.cs # src/Examine.AzureDirectory/Properties/AssemblyInfo.cs # src/Examine/Properties/AssemblyInfo.cs

…ons. Refactor to simplify code/ improve code reuse. Change framework to 4.6.1 for azuredirectory, test and demo projects as required by blob storage package

…h namespace. Integration test is not very reliable with the azure storage emulator.

bielu · 2020-12-04T13:24:34Z

@nzdev Great Job about update of blob provider, but in same Time I dont really agree we should include logger, as Examine is low API abstraction, and logging should be on layer which use that, not inside of low level abstraction :)

nzdev · 2020-12-04T21:12:10Z

Thanks for the feedback @bielu. I hope we can work together to get azuredirectory working well 😁.

I do think adding logging is appropriate. I've made it so it's opt in to provide the logger instance. However I believe it's important to have logging within this project as the exceptions are handled within this layer. Without logging at this layer it's going to be difficult to know what happens when things go wrong.

Shazwazza · 2020-12-15T01:44:09Z

I have reviewed #197 and seems to me that those changes will need to be reviewed and merged before I can look at this one? There's a boat load of changes here and just don't want to duplicate a lot of work. Are you guys ok with that? I'll focus on #197 first before this?

nzdev · 2020-12-15T02:28:15Z

@Shazwazza I've done a bit of refactoring in this branch to clean things up, but any logic issues/ comments required would be better in the other branch. I'm not sure if there would need to be two releases of azure directory as .net4.5 is not supported in the current azure sdk.

Shazwazza · 2020-12-15T04:45:35Z

azure directory hasn't been released for v1 so we can do whatever needs to be done :) as for back porting it to 0.x that may be a diff story but we'll keep these changes for v1.

nzdev · 2020-12-15T05:27:46Z

In that case @Shazwazza I would recommend going with this branch as I've cleaned up and fixed a few things

bielu · 2020-12-15T22:10:17Z

@nzdev I am actually also doing cleanup and refactors and now we can finish with total different outcomes :), Can you send same PR to me and then I will merge codes and we will finish with PR instead of 2, which will make job for @Shazwazza much easier?

nzdev · 2020-12-15T22:43:13Z

If you are on .net 4.6. @bielu It may be worth branching off this branch instead as it's already cleaned up and fixes some of the things pointed out by @Shazwazza. Otherwise, happy to send a pr your way too.

bielu · 2020-12-15T23:23:16Z

Will look over weekend into that :)

src/Examine.AzureDirectory/AzureLuceneDirectory.cs

src/Examine.AzureDirectory/AzureReadOnlyLuceneDirectory.cs

bielu · 2020-12-18T13:15:22Z

@nzdev I just went through your fork of mine fork, and I am going to comment there a lot off.

bielu

There is few comments which I had time to prepare, I still think idea of manifest is wrong or least wrongly implement. In loadbalanced environment you dont want be checking on flag, as it will cause delays, my approach with checking on that on request, causing that to be almost realtime.
I am going to build beta package from that branch and check if it is working and not breaking on loadbalanced environment and compare how much difference there is on time of reaction.

src/Examine/LuceneEngine/Directories/ExamineDirectory.cs

bielu · 2020-12-18T13:16:49Z

src/Examine/LuceneEngine/Directories/ExamineDirectory.cs

+        }
+        public Lucene.Net.Store.Directory CacheDirectory { get; protected set; }
+
+        public abstract string[] CheckDirtyWithoutWriter();


same as above

bielu · 2020-12-18T13:16:55Z

src/Examine/LuceneEngine/Directories/ExamineDirectory.cs

+
+        public abstract string[] CheckDirtyWithoutWriter();
+
+        public abstract void SetDirty();


same as above

bielu · 2020-12-18T13:18:58Z

src/Examine/LuceneEngine/ExamineIndexWriter.cs

+    public class ExamineIndexWriter : IndexWriter
+    {
+        private ExamineDirectory _examineDirectory;
+        public ExamineIndexWriter(Directory d, Analyzer a, MaxFieldLength mfl) : base(d, a, mfl)


I don't think so that creation make sense.

src/Examine/LuceneEngine/Providers/LuceneIndex.cs

bielu · 2020-12-18T13:19:42Z

src/Examine/LuceneEngine/Providers/LuceneIndex.cs

@@ -78,13 +93,26 @@ public class LuceneIndex : BaseIndexProvider, IDisposable, IIndexStats
            DefaultAnalyzer = analyzer ?? new StandardAnalyzer(Lucene.Net.Util.Version.LUCENE_30);

            _directory = luceneDirectory;
+            if (luceneDirectory is ExamineDirectory dir)


Same as above

src/Examine/LuceneEngine/Providers/LuceneIndex.cs

bielu · 2020-12-18T13:20:42Z

src/Examine/LuceneEngine/Providers/LuceneIndex.cs

@@ -422,7 +469,14 @@ public void EnsureIndex(bool forceOverwrite)

                                //remove all of the index data
                                _writer.DeleteAll();
-                                _writer.Commit();
+                                if(_writer is ExamineIndexWriter examineIndexWriter)


Still dont think so idea of Custom writer and manifest make sense at all.

bielu · 2020-12-18T13:26:02Z

src/Examine/LuceneEngine/Providers/LuceneIndex.cs

        }

+        public override void DeleteFromIndex(IEnumerable<string> itemIds)


We should canceled operation in OnDocumentWriting instead.

bielu · 2020-12-18T13:27:01Z

src/Examine/Providers/BaseIndexProvider.cs

@@ -71,13 +71,13 @@ protected ValueSetValidationResult ValidateItem(ValueSet item)
        /// Validates the items and calls <see cref="M:Examine.Providers.BaseIndexProvider.PerformIndexItems(System.Collections.Generic.IEnumerable{Examine.ValueSet})" />
        /// </summary>
        /// <param name="values"></param>
-        public void IndexItems(IEnumerable<ValueSet> values)
+        public virtual void IndexItems(IEnumerable<ValueSet> values)


If we will do as comments above we dont need override that methods, but I agree they should be virtual anyway :)

Shazwazza · 2021-01-05T23:53:16Z

Hi all, sorry haven't had time to follow up on this just yet (still getting back from holiday mode!)

But I had a thought and I'm unsure what the solution is. The https://www.nuget.org/packages/UmbracoFileSystemProviders.Azure/ media blob storage package uses the v11 version. Since this is quite a populate package and the only non-cloud official media blob storage package around we need to ensure that things work side by side.

I'm unsure if the v11 and v12 packages can co-exist, anyone know? Else there's an ugly ILMerge route that could be taken, else I think that media blob package will need to be updated too and this could only exist alongside that newer version.

Thoughts?

bielu · 2021-01-06T00:15:01Z

@Shazwazza v11 and v12 from that what I know can coexist, but it is not recommend and we should probably help update package to not use deprecated apis anyway. (It is based on that they are in different namespace(with and without Microsoft)
anyway we still waiting for @nzdev to review my pr to his pr :)
https://github.com/nzdev/Examine/pull/1/files

nzdev · 2021-01-09T05:50:55Z

Yes. umbraco-community/UmbracoFileSystemProviders.Azure#179

bielu · 2021-01-16T12:34:37Z

@Shazwazza after few messages with @nzdev we figure out the best way of handling all providers for remote directory is to abstract out APIs for Azure/S3 etc can you maybe say which approach you prefer to do move abstractions?

Move Renamed Dir to main Examine Package and later in Examine.AzureDirectoryhave only parts needed for azure?
Separate abstraction to 3 package Examine.RemoteDir and use that as depedency in Examine.AzureDirectory

Shazwazza · 2021-01-21T05:17:23Z

Hi all,

can you maybe say which approach you prefer to do move abstractions?

At this stage I think you both know more about the requirements then I do since my head hasn't been in this part of the codebase as much as you have recently. Having a separate 3rd package seems like a 'cleaner' solution and also means that if changes are needed within that package it can be deployed independently of an Examine version. But happy to take your lead, what do you think?

Shazwazza

Have left lots of comments/questions inline. I'm unsure about the ExamineIndexWriter change. It seems that all logic that this is dealing with really has nothing to do with the default LuceneEngine. Things like dirty checking, etc... are all to do with specific implementations of LuceneIndex. Seems like it would make more sense to have an inherited class deal with that stuff that is specific to 'Remote' directories.

Shazwazza · 2021-01-24T22:54:21Z

src/Examine.AzureDirectory/AzureIndexOutput.cs

+                    catch(Azure.RequestFailedException ex) when (ex.Status == 409)
+                    {
+                        //File already exists
+                        throw;


Is there a reason to catch this and then just re-throw? Or is this just for debugging if breakpointing?

Shazwazza · 2021-01-24T22:57:49Z

src/Examine.AzureDirectory/AzureLuceneDirectory.cs


+        public Lucene.Net.Store.Directory CacheDirectory { get; protected set; }


Should this be protected set? This is a ctor dependency so should be readonly?

@Shazwazza it can't be readonly as it is used an swapped by reaonly, it is why it is protected. if you check there logic:

Examine/src/Examine.AzureDirectory/AzureReadOnlyLuceneDirectory.cs

Line 57 in c4f9483

CacheDirectory = new SimpleFSDirectory(directory);

To avoid corruption of indexes :)

src/Examine.AzureDirectory/AzureLuceneDirectory.cs

Shazwazza · 2021-01-24T23:06:29Z

src/Examine.AzureDirectory/AzureLuceneDirectory.cs

+            Trace.WriteLine($"INFO Syncing file {fileName} for {RootFolder}");
+            // then we will get it fresh into local deflatedName 
+            // StreamOutput deflatedStream = new StreamOutput(CacheDirectory.CreateOutput(deflatedName));
+            using (var deflatedStream = new MemoryStream())


deflatedStream here isn't the correct name, it's just stream that the blob is downloaded too. I realize this was a copy/paste from other code but should be renamed, even just memStream or whatever is fine.

Shazwazza · 2021-01-24T23:07:17Z

src/Examine.AzureDirectory/AzureLuceneDirectory.cs

            blob.DeleteIfExists();
            SetDirty();

-            Trace.WriteLine($"DELETE {_blobContainer.Uri}/{name}");
+            Trace.WriteLine($"INFO Deleted { _blobContainer.Uri}/{name} for {RootFolder}");


Do we need 2x trace here?

Shazwazza · 2021-01-24T23:47:00Z

src/Examine/LuceneEngine/Providers/LuceneIndex.cs

@@ -370,7 +392,7 @@ protected override void PerformIndexItems(IEnumerable<ValueSet> values, Action<I
        public void EnsureIndex(bool forceOverwrite)
        {
            if (!forceOverwrite && _exists.HasValue && _exists.Value) return;
-
+            if (_directory is ExamineDirectory examineDirectory && examineDirectory.IsReadOnly) return;


if we keep this idea of ExamineDirectory then this check should be encapsulated in a protected property IsReadOnly since this same check if (_directory is ExamineDirectory examineDirectory && examineDirectory.IsReadOnly) return; is done a few times.

@Shazwazza as pointed earlier I am not sure either about that :) but didnt removed that from my pr to this pr. :)

Shazwazza · 2021-01-24T23:49:33Z

src/Examine/LuceneEngine/Providers/LuceneIndex.cs

+                    if(_directory is ExamineDirectory examineDirectory)
+                    {
+                        //Calling commit causes fdt,fdx,fnm,frq,nrm,prx,tii,tis,tvd,tvf,tvx files to be deleted.
+                        //TODO: 


Is this code comment still relevant? Commit shouldn't remove files IIRC, that is done when merging occurs

Shazwazza · 2021-01-24T23:50:10Z

src/Examine/LuceneEngine/Providers/LuceneIndex.cs

@@ -1031,7 +1100,14 @@ public void ScheduleCommit()
                        if (_index._cancellationTokenSource.IsCancellationRequested)
                        {
                            //perform the commit
-                            _index._writer?.Commit();
+                            if (_index._writer is ExamineIndexWriter examineIndexWriter)


This same check is performed a lot, would be better to encapsulate this logic so there's no duplication.

Shazwazza · 2021-01-24T23:51:31Z

src/Examine/LuceneEngine/Providers/LuceneIndex.cs

                {
+                    //TODO: if readonly index, unlock the local index?


I would assume so yes but the IndexWriter in that case should already just be the local index?

Shazwazza · 2021-01-24T23:54:33Z

src/Examine/LuceneEngine/Providers/LuceneSearcher.cs

+                                }
+                                catch (Exception e)
+                                {
+                                    //It's the initial call to this at the beginning or after successful commit


This try/catch seems significant, was this caused somewhere during debugging? Should this try/catch be ported individually to another PR?

@Shazwazza after refactor it is actually redundant as it was happening only in readonly, but I changed way of handling read only to never occur again, but left that as in time of debugging I hit issue when old reader was already disposed, and new one was not ready yet, and adding that try catch up was fixing about 10-15% of my crashes during debugging :)

bielu · 2021-01-25T01:04:17Z

@Shazwazza some of that comments will be already addressed if @nzdev will merge my PR to this PR :)
https://github.com/nzdev/Examine/pull/1/files

Arkadiusz Biel and others added 25 commits November 8, 2020 00:02

POC of nomerge, no write for readonly event for v1

688b287

add more security on missing files

a5f09bf

ver

dbd12f1

Merge remote-tracking branch 'github.com/v1.0.6' into bugfix/azure-bl…

16c3429

…ob-v1 # Conflicts: # build/build.xml

semi working ver

0f58a0e

make no merge sheduler public

c4edc82

create proper readonly DIrectory with aliasing for time of resync

e342c4a

correct versions

e53d62a

Merge remote-tracking branch 'github.com/master' into bugfix/azure-bl…

d439b77

…ob-v1 # Conflicts: # src/Examine.AzureDirectory/AzureDirectory.cs # src/Examine.AzureDirectory/Properties/AssemblyInfo.cs # src/Examine/Properties/AssemblyInfo.cs

move definition to new interface

62f0fb9

Separate method from new interface to new Factory

23257d7

removed unsed code and definitions

1ccbdec

correct definition

5ebc18e

correct nuspec and directory

35afc28

Fix ruleset so this builds

9ee49ac

Update blob storage package as previous obsolete. Add logging extensi…

be6ccb7

…ons. Refactor to simplify code/ improve code reuse. Change framework to 4.6.1 for azuredirectory, test and demo projects as required by blob storage package

Add logging

fb79db7

Deduplicate isreadonly

2993a68

don't change name unintentionally

2b60857

logging for lock errors

0d80363

Renamed AzureDirectory to AzureLuceneDirectory to avoid conflicts wit…

9fb1f56

…h namespace. Integration test is not very reliable with the azure storage emulator.

revert overwrite

b7af836

Tests run (with possible bugs) in azurite

66a16d9

allow overwrite

8c94405

More extensibility

df80409

nzdev added 3 commits December 5, 2020 23:04

update nuspec

e0f5504

update nuspec dependencies

7e65b23

bugfix. update nuspec

abd1216

nzdev added 5 commits December 7, 2020 10:50

change version number

761826b

just needs to stop writing the .lock file in the readonly cache

a636d95

test passes

b6043f2

Fix test

4d119d5

wip, use a manifest

3f0ea94

nzdev added 2 commits December 15, 2020 19:23

disable writes for readonly indexes. Fix decompression.

0e469fb

fix documentwriting event

5f069ce

bielu reviewed Dec 16, 2020

View reviewed changes

src/Examine.AzureDirectory/AzureLuceneDirectory.cs Show resolved Hide resolved

src/Examine.AzureDirectory/AzureReadOnlyLuceneDirectory.cs Outdated Show resolved Hide resolved

src/Examine.AzureDirectory/AzureReadOnlyLuceneDirectory.cs Outdated Show resolved Hide resolved

bielu reviewed Dec 18, 2020

View reviewed changes

nzdev added 2 commits December 21, 2020 20:29

Remove cachedirectory from examinedirectory

7086766

remove commented out code

8f1431c

nzdev added 3 commits January 9, 2021 14:43

merge

805b5c7

test fix

311f347

Fix lock name (bielu). Catch exception for empty old sync dir

c4f9483

Shazwazza requested changes Jan 24, 2021

View reviewed changes

nzdev closed this Jun 23, 2021


		public abstract string[] CheckDirtyWithoutWriter();

		public abstract void SetDirty();

		}

		public override void DeleteFromIndex(IEnumerable<string> itemIds)


		public Lucene.Net.Store.Directory CacheDirectory { get; protected set; }

Update Azure Storage SDK to v12, add logging for Azure Blob storage directory #198

Update Azure Storage SDK to v12, add logging for Azure Blob storage directory #198

Conversation

nzdev commented Dec 4, 2020 • edited Loading

bielu commented Dec 4, 2020

nzdev commented Dec 4, 2020

Shazwazza commented Dec 15, 2020

nzdev commented Dec 15, 2020

Shazwazza commented Dec 15, 2020

nzdev commented Dec 15, 2020

bielu commented Dec 15, 2020

nzdev commented Dec 15, 2020

bielu commented Dec 15, 2020

bielu commented Dec 18, 2020

bielu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shazwazza commented Jan 5, 2021

bielu commented Jan 6, 2021 • edited Loading

nzdev commented Jan 9, 2021

bielu commented Jan 16, 2021

Shazwazza commented Jan 21, 2021

Shazwazza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bielu commented Jan 25, 2021

nzdev commented Dec 4, 2020 •

edited

Loading

bielu commented Jan 6, 2021 •

edited

Loading