Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DatabaseTarget - Enable OptimizeBufferReuse by default #1908

Merged
merged 1 commit into from
Jun 23, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Jan 13, 2017

Waiting for NLog ver. 4.5


This change is Reviewable

@304NotModified
Copy link
Member

NLog.UnitTests.Targets.Wrappers.BufferingTargetWrapperTests.BufferingTargetWrapperAsyncTest1 [FAIL]
Assert.Equal() Failure
Expected: 19
Actual: 18
Stack Trace:
at NLog.UnitTests.Targets.Wrappers.BufferingTargetWrapperTests.BufferingTargetWrapperAsyncTest1 () [0x001e6] in <50fe24b3f1d14d7092cf9b8d5f6ecaaa>:0
at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00038] in <8f2c484307284b51944a1a13a14c0266>:0
1922 total, 1 failed, 21 skipped, took 50.686 seconds

Seems a fail from a recent change. Is that possible?

@304NotModified
Copy link
Member

Reviewed 1 of 2 files at r1.
Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/NLog/Targets/DatabaseTarget.cs, line 93 at r1 (raw file):

            this.ConnectionStringsSettings = ConfigurationManager.ConnectionStrings;
            this.CommandType = CommandType.Text;
            this.OptimizeBufferReuse = GetType() == typeof(DatabaseTarget);

what's the use of this? Not enabling in subclasses?


src/NLog/Targets/DatabaseTarget.cs, line 662 at r1 (raw file):

                    this.EnsureConnectionOpen(cs);

                    using (var command = this.activeConnection.CreateCommand())

behavior change?


Comments from Reviewable

@snakefoot
Copy link
Contributor Author

Seems a fail from a recent change. Is that possible?

Looks more like a bad unit test. There is no guarantee that the write-events will complete before the flush-event as MyAsyncTarget is using ThreadPool.QueueUserWorkItem.

So the the flushHit-event can be triggered before everything is written, thus causing the problem with expecting 19 but only 18 written.

Maybe MyAsyncTarget should apply the newly created AsyncOperationCounter :). Like it has been done in AsyncTargetWrapperTests.cs

@codecov-io
Copy link

codecov-io commented Jan 19, 2017

Codecov Report

Merging #1908 into master will increase coverage by <1%.
The diff coverage is 74%.

@@           Coverage Diff           @@
##           master   #1908    +/-   ##
=======================================
+ Coverage      81%     81%   +<1%     
=======================================
  Files         285     285            
  Lines       19436   19441     +5     
  Branches     2263    2263            
=======================================
+ Hits        15823   15837    +14     
- Misses       3060    3066     +6     
+ Partials      553     538    -15

@304NotModified
Copy link
Member

Reviewed 1 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@304NotModified 304NotModified merged commit 3da08f7 into NLog:master Jun 23, 2017
@304NotModified 304NotModified modified the milestones: 4.5, 4.5 beta ? Oct 10, 2017
@snakefoot snakefoot deleted the DatabaseOptimizeBufferReuse branch October 10, 2017 20:42
@snakefoot snakefoot mentioned this pull request Nov 23, 2017
@snakefoot
Copy link
Contributor Author

@304NotModified 304NotModified added the documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) label Jan 14, 2018
@snakefoot snakefoot modified the milestones: 4.5 beta ?, 4.5 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database-target documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) needs documentation on wiki performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants