Change thrift namespace to avoid conflicts #87

Closed
tjake opened this Issue Nov 20, 2012 · 17 comments

Projects

None yet

4 participants

@tjake
tjake commented Nov 20, 2012

fluentcassandra embeds thrift (not sure why)

It conflicts with our nuget version since the namespaces collide. It would be polite to change your copied source code to use a diff namespace to avoid this conflict

@nberardi
Contributor

It embeds thrift because it needs thrift to function. We cannot change
this.

On Nov 20, 2012, at 12:46 PM, Jake Luciani notifications@github.com wrote:

fluentcassandra embeds thrift (not sure why)

It conflicts with our nuget version since the namespaces collide. It would
be polite to change your copied source code to use a diff namespace to
avoid this conflict


Reply to this email directly or view it on
GitHubhttps://github.com/managedfusion/fluentcassandra/issues/87.

@nberardi
Contributor

What is your NuGet version?

@tjake
tjake commented Nov 20, 2012

I mean why can't it use a external thrift.dll

@nberardi
Contributor

To be honest. It is because the maintainer doesn't keep on top of the
updates. 0.9 has been out for a while now. Over a month now. And we are
just getting the 0.9 release.

Nick Berardi
(484) 302-0125
Sent on the go from my phone.

On Nov 20, 2012, at 12:50 PM, Jake Luciani notifications@github.com wrote:

https://nuget.org/packages/Thrift


Reply to this email directly or view it on
GitHubhttps://github.com/managedfusion/fluentcassandra/issues/87#issuecomment-10565075.

@tjake
tjake commented Nov 20, 2012

There's actually a C# bug in 0.9 release, I don't see any reason why you should be using 0.9 over 0.8

@nberardi
Contributor

The fact their is a bug is all the better reason for the control to remain
in FluentCassandra.

Nick Berardi
(484) 302-0125
Sent on the go from my phone.

On Nov 20, 2012, at 1:28 PM, Jake Luciani notifications@github.com wrote:

There's actually a C# bug in 0.9 release, I don't see any reason why you
should be using 0.9 over 0.8


Reply to this email directly or view it on
GitHubhttps://github.com/managedfusion/fluentcassandra/issues/87#issuecomment-10566554.

@eplowe
Contributor
eplowe commented Nov 20, 2012

Its also been marked as closed. I tend to agree with Nick that
fluentcassandra a imported version of thrift is a good thing vs linking
against an external version. It gives us more control.
On Nov 20, 2012 1:32 PM, "Jake Luciani" notifications@github.com wrote:

https://issues.apache.org/jira/browse/THRIFT-1709


Reply to this email directly or view it on GitHubhttps://github.com/managedfusion/fluentcassandra/issues/87#issuecomment-10566692.

@tjake
tjake commented Nov 20, 2012

The bug is actually in the 0.9 release they reverted it post release if you look at the history.

@carlyeks

You could break out the thrift into its own library so that others can link to the thrift library as well as fluentcassandra. As it stands now, any user of fluentcassandra must use the thrift included (not a separate library since the namespaces have been polluted), but fluentcassandra should not be enforcing that. By linking to a separate fluentcassandra fork of the thrift library, you maintain control of the version of thrift you are using, but don't force others to use your version (since including a different version of the thrift library will do the same).

@nberardi
Contributor

I will consider it, but as of right now I have no plans to link to the Thrift library in NuGet.

@eplowe
Contributor
eplowe commented Nov 20, 2012

Jake: unless I am misunderstanding this:

Reverts initial patch submitted with ticket and changes to using unchecked
{} due to reported memory issues with the previous patch. (Revision 1400487)

And issue has been marked fixed. I take that it has been resolved. The
first attempt cause OOM issues the second was to wrap it in an unchecked
block. It also affects version .8 as well. The initial complaint was the
compiler warning.
On Nov 20, 2012 2:18 PM, "Jake Luciani" notifications@github.com wrote:

The bug is actually in the 0.9 release they reverted it post release if
you look at the history.


Reply to this email directly or view it on GitHubhttps://github.com/managedfusion/fluentcassandra/issues/87#issuecomment-10568633.

@tjake
tjake commented Nov 20, 2012

The initial fix to the issue was to change the logic to using unsigned longs which caused huge memory spiked to to a bug in the implementation. This made them revert the change but the official 0.9.0 still contains the broken code.

@eplowe
Contributor
eplowe commented Nov 20, 2012

I am seeing the use of an unchecked block. Which was used after the oom
issues you described. The bug, which is a compiler warning, also exists in
.8 as well
On Nov 20, 2012 2:32 PM, "Jake Luciani" notifications@github.com wrote:

The initial fix to the issue was to change the logic to using unsigned
longs which caused huge memory spiked to to a bug in the implementation.
This made them revert the change but the official 0.9.0 still contains the
broken code.


Reply to this email directly or view it on GitHubhttps://github.com/managedfusion/fluentcassandra/issues/87#issuecomment-10569182.

@nberardi
Contributor

For the time being, I am going to rename the Thrift namespace. This should get around the issue described with conflicts. I don't want to take on a reference in NuGet at this moment, because it would complicate things for people not using NuGet.

@nberardi nberardi was assigned Nov 20, 2012
@nberardi nberardi closed this Nov 20, 2012
@tjake
tjake commented Nov 20, 2012

Great thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment