SERVER-9751 Force S2 geometry library to use Mongo's LOG instead of stderr #430

svetlyak40wt · 2013-05-22T07:07:50Z

Right now S2 library just writes any debugging output and errors
to std::cerr. This patch changes S2's logger to use Mongo's LOG macros.

That way it is much easier to find errors in geometry primitives.

…std::cerr. Right now S2 library just writes any debugging output and errors to std::cerr. This patch changes S2's logger to use Mongo's LOG macros. That way it is much easier to find errors in geometry primitives.

kangas · 2013-05-22T20:19:40Z

Hi Alexander,

Before we can accept any pull request, we have two main requirements. (1) reference a SERVER ticket, (2) make sure you've signed the contributor agreement. This is explained in CONTRIBUTING.rst in the root of our source tree.

I see you already have the SERVER ticket taken care of, so that's great! Now we need the contributor agreement. The form is at http://www.10gen.com/legal/contributor-agreement

Even though it's not "required" in the form, please make sure you specify your GitHub username. This will help us verify compliance quickly in the future.

Thank you for contributing to MongoDB!

svetlyak40wt · 2013-05-23T12:06:40Z

Hi, I already did it yesterday.

kangas · 2013-05-23T14:38:36Z

Hi Alexander,

Unfortunately I do not see you on our contributors list yet. Can you fill out the form again? It's possible the problem is on our end. If you tell me you submitted the form a second time and I still see nothing, I will be happy to hunt for a problem here.

Again, please specify your GitHub username, even though the form doesn't require it (yet).

svetlyak40wt · 2013-05-23T14:56:19Z

Done

kangas · 2013-05-23T19:38:13Z

Ok. I still don't see your name on the contributors list, but I will assume the problem is on our end. Let me work on this.

svetlyak40wt · 2013-05-24T03:46:30Z

Well, here is a screenshot.

And also I've got an email, saying "10gen/MongoDB Contributor Agreement (between Yandex and 10gen Inc) is Signed and Filed!". Agreement's id is UA344Z5E5J3GXZ.

kangas · 2013-05-24T12:40:19Z

Alexander, I've confirmed that we have a problem on our end. I'm trying to
get it resolved. Until then, your screenshot looks good. I will pass your
request along to someone who can review it on technical merits. Thank you
for contributing!
On May 23, 2013 11:46 PM, "Alexander Artemenko" notifications@github.com
wrote:

Well, here is a screenshot.
[image: screen shot 2013-05-23 at 18 56 08]https://f.cloud.github.com/assets/24827/558570/22076d7e-c424-11e2-8a6d-338fc78c778e.png

And also I've got an email, saying "10gen/MongoDB Contributor Agreement
(between Yandex and 10gen Inc) is Signed and Filed!". Agreement's id is
UA344Z5E5J3GXZ.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/430#issuecomment-18385160
.

hkhalsa · 2013-05-29T15:14:02Z

src/third_party/s2/base/logging.h


 // Always-on checking
-#define CHECK(x)	if(x){}else LogMessageFatal(__FILE__, __LINE__).stream() << "Check failed: " #x
+#define CHECK(x)	if(x){}else VLOG(2) << __FILE__ << ":" << __LINE__ << ": Check failed: " #x


We need CHECK to still be fatal, especially in non-debug mode. Maybe a verify(x) after the vlog?

I think we want:
error() << FILE << .. (and so on) verify(x);
when the check fails.

Well, original CHECK is not fatal. Do you really think this should be changed?

And I'm not familiar with MongoDB's codebase, what does verify do?

It isn't? ~LogMessageFatal calls ::abort() which should stop the server. Did you see different behavior in practice?

Verify stops the server if the condition is false and prints out a stack trace/other info.

Well, I've read assert_util.h and think that this line should be replaced with just:

#define CHECK MONGO_verify

Do you agree with me?

I agree. Thanks for looking into this. Should be good to pull once that change is made.

Verify is thread-fatal, while fassert is process-fatal. Is the process
compromised after CHECK failures in S2? If so, fassert.
On Jun 5, 2013 1:37 PM, "Hari Khalsa" notifications@github.com wrote:

In src/third_party/s2/base/logging.h:

// Always-on checking
-#define CHECK(x) if(x){}else LogMessageFatal(FILE, LINE).stream() << "Check failed: " #x
+#define CHECK(x) if(x){}else VLOG(2) << FILE << ":" << LINE << ": Check failed: " #x

I agree. Thanks for looking into this. Should be good to pull once that
change is made.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/430/files#r4550584
.

@andy10gen original behaviour was to call abort() if check failed.

Well, guys, I found that CHECK can't be just redefined as a MONGO_verify. Because originally, CHECK returns an object to be used as a stream for error message.

What should be used in this case? Probably MONGO_massert? If yes, then what should I use as msgid?

Well, I had intended to make all of the assertions streamable, a la
Google's CHECK, eventually, but never got around to it. I'm working on
making at least the fatal asssertions (fassert) streamable, and will
respond here and on SERVER-9751 when I do. Should be sometime in the next
week to two, after I get a few other things taken care of.

On Thu, Jun 6, 2013 at 3:53 AM, Alexander Artemenko <
notifications@github.com> wrote:

In src/third_party/s2/base/logging.h:

// Always-on checking
-#define CHECK(x) if(x){}else LogMessageFatal(FILE, LINE).stream() << "Check failed: " #x
+#define CHECK(x) if(x){}else VLOG(2) << FILE << ":" << LINE << ": Check failed: " #x

Well, guys, I found that CHECK can't be just redefined as a MONGO_verify.
Because originally, CHECK returns an object to be used as a stream for
error message.

What should be used in this case? Probably MONGO_massert? If yes, then
what should I use as msgid?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/430/files#r4562525
.

kangas · 2014-01-22T22:44:27Z

Alexander, sorry it took so long, but we finally resolved this in 8af9af8. Thank you for your patience and for contributing to MongoDB!

yojique · 2014-07-31T14:34:32Z

Hello.
What is the way to turn s2 log messages in 2.6.3 ?
Seems like '-vvvvv' doesn't work and i just see standard 16755 error description

benety · 2014-08-07T14:37:34Z

Hi Andrii,

We redirected the non-debug S2 logs in SERVER-9751. We're currently working on the debug S2 messages in SERVER-14467. Once SERVER-14467 is resolved, you should be able to see in a development release the S2 messages with the details on a query with an invalid geometry.

Ben

yojique · 2014-08-07T19:51:12Z

Thanks Ben.
I'm really looking forward for this.

benety · 2014-08-13T15:36:43Z

Hi Andrii,

We resolved SERVER-14467 recently so you should see this in the upcoming 2.7.5 development release. Here's some sample logs (at verbosity 5) from running the JS test geo_invalid_polygon.js:

% buildscripts/smoke.py --set-parameters=logLevel=5  --mode=files jstests/core/geo_invalid_polygon.js
2014-08-13T11:15:42.317-0400 I          [initandlisten] MongoDB starting : pid=21518 port=27999 dbpath=/data/db/sconsTests/ 64-bit host=myhost.local
...
2014-08-13T11:16:01.090-0400 D S2       [conn3] Edges 0 and 2 cross
2014-08-13T11:16:01.090-0400 D S2       [conn3] Edge locations in degrees: -1.000000,-2.000000-1.000000,1.000000 and 1.000000,0.000000-0.000000,0.000000
2014-08-13T11:16:01.090-0400 D          [conn3] User Assertion: 16755:Can't extract geo keys from object, malformed geometry?: { _id: 42.0, geometry: { type: "Polygon", coordinates: [ [ [ 0.0, 0.0 ], [ 0.0, 1.0 ], [ 1.0, 1.0 ], [ -2.0, -1.0 ], [ 0.0, 0.0 ] ] ] } }
...
2014-08-13T11:16:01.169-0400 I STORAGE  [signalProcessingThread] shutdown: closing all files...
2014-08-13T11:16:01.169-0400 I STORAGE  [signalProcessingThread] closeAllFiles() finished

Regards,
Ben

yojique · 2014-08-14T10:00:55Z

Thanks Ben.

yojique · 2014-08-14T13:16:07Z

Hello Ben.
I have an improvement proposal described here
https://jira.mongodb.org/browse/SERVER-14894

Thanks.

Disable forced eviction if eviction is disabled for the whole tree.

hkhalsa reviewed May 29, 2013
View reviewed changes

kangas closed this Jan 22, 2014

benety changed the title ~~SERVER-9751: Force S2 geometry library to use Mongo's LOG instead of stderr~~ SERVER-9751 Force S2 geometry library to use Mongo's LOG instead of stderr Aug 1, 2014

jiongle1 pushed a commit to scantist-ossops-m2/mongo that referenced this pull request Mar 30, 2024

Merge pull request mongodb#430 from wiredtiger/evict-force-lsm

c70841b

Disable forced eviction if eviction is disabled for the whole tree.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SERVER-9751 Force S2 geometry library to use Mongo's LOG instead of stderr #430

SERVER-9751 Force S2 geometry library to use Mongo's LOG instead of stderr #430

svetlyak40wt commented May 22, 2013

kangas commented May 22, 2013

svetlyak40wt commented May 23, 2013

kangas commented May 23, 2013

svetlyak40wt commented May 23, 2013

kangas commented May 23, 2013

svetlyak40wt commented May 24, 2013

kangas commented May 24, 2013

hkhalsa May 29, 2013

svetlyak40wt May 29, 2013

hkhalsa May 29, 2013

svetlyak40wt May 29, 2013

hkhalsa Jun 5, 2013

amschwerin Jun 5, 2013

svetlyak40wt Jun 6, 2013

svetlyak40wt Jun 6, 2013

amschwerin Jun 8, 2013

kangas commented Jan 22, 2014

yojique commented Jul 31, 2014

benety commented Aug 7, 2014

yojique commented Aug 7, 2014

benety commented Aug 13, 2014

yojique commented Aug 14, 2014

yojique commented Aug 14, 2014

SERVER-9751 Force S2 geometry library to use Mongo's LOG instead of stderr #430

SERVER-9751 Force S2 geometry library to use Mongo's LOG instead of stderr #430

Conversation

svetlyak40wt commented May 22, 2013

kangas commented May 22, 2013

svetlyak40wt commented May 23, 2013

kangas commented May 23, 2013

svetlyak40wt commented May 23, 2013

kangas commented May 23, 2013

svetlyak40wt commented May 24, 2013

kangas commented May 24, 2013

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

kangas commented Jan 22, 2014

yojique commented Jul 31, 2014

benety commented Aug 7, 2014

yojique commented Aug 7, 2014

benety commented Aug 13, 2014

yojique commented Aug 14, 2014

yojique commented Aug 14, 2014