Skip to content

Conversation

@stoty
Copy link
Contributor

@stoty stoty commented Apr 27, 2022

No description provided.

@virajjasani
Copy link
Contributor

Doesn't seem relevant to this PR but I see lots of refCount leaked errors in DateTimeIT

@virajjasani
Copy link
Contributor

Otherwise +1 for dropping phoenix-client and making only embedded jar available (target version only 5.2.x)

Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but one question about phoenix-tools

</activation>
<modules>
<module>phoenix-client-parent</module>
<module>phoenix-client-parent/phoenix-client</module>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop phoenix-client-parent/phoenix-client directory and pom.xml too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could.
However, I plan to have at least one more variant that that can coexist hbase-shaded-client (PHOENIX-6053), and it's easier to keep the structure than to remove and re-add it later.
If you feel strongly about it, then we certainly can remove this.

Copy link
Contributor Author

@stoty stoty Apr 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you mean just the phoenix-client directory.
Sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@stoty
Copy link
Contributor Author

stoty commented Apr 29, 2022

Doesn't seem relevant to this PR but I see lots of refCount leaked errors in DateTimeIT

Yes, we have a lot of those lately (we get that in our downstream tests, too).
I haven't been able to look into that, but it would be great to solve those sooner than later.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants