You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@fluo.apache.org by mjwall <gi...@git.apache.org> on 2017/05/09 12:22:35 UTC
[GitHub] incubator-fluo pull request #832: closes #809 - cleanup buffered data
GitHub user mjwall opened a pull request:
https://github.com/apache/incubator-fluo/pull/832
closes #809 - cleanup buffered data
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mjwall/incubator-fluo 809-dereference-buffered-data
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-fluo/pull/832.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #832
----
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo issue #832: closes #809 - cleanup buffered data
Posted by mjwall <gi...@git.apache.org>.
Github user mjwall commented on the issue:
https://github.com/apache/incubator-fluo/pull/832
same errors with mvn verify locally, so I'll spend some time trying to understand what is failing
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #832: closes #809 - cleanup buffered data
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/832#discussion_r115614639
--- Diff: modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java ---
@@ -596,9 +596,17 @@ public CommitData createCommitData() {
@Override
public synchronized void commit() throws CommitException {
- SyncCommitObserver sco = new SyncCommitObserver();
- commitAsync(sco);
- sco.waitForCommit();
+ SyncCommitObserver sco = null;
+ try {
+ sco = new SyncCommitObserver();
+ commitAsync(sco);
+ sco.waitForCommit();
+ } finally {
+ updates.clear();
+ weakNotification = null;
+ observedColumns.clear();
--- End diff --
The way I found the bug was by commenting out all of the clears except for one and running workerIT. Sorry for suggesting to clear that, I wasn't thinking about it too much I just looked at all of the instance vars looking for maps and sets.
I opened #833.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo issue #832: closes #809 - cleanup buffered data
Posted by mjwall <gi...@git.apache.org>.
Github user mjwall commented on the issue:
https://github.com/apache/incubator-fluo/pull/832
thanks @keith-turner, I'll make a PR like you suggested for the contributors page
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo issue #832: closes #809 - cleanup buffered data
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the issue:
https://github.com/apache/incubator-fluo/pull/832
ok, I will pull the branch down and take a look at it too
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #832: closes #809 - cleanup buffered data
Posted by mjwall <gi...@git.apache.org>.
Github user mjwall commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/832#discussion_r115620114
--- Diff: modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java ---
@@ -596,9 +596,17 @@ public CommitData createCommitData() {
@Override
public synchronized void commit() throws CommitException {
- SyncCommitObserver sco = new SyncCommitObserver();
- commitAsync(sco);
- sco.waitForCommit();
+ SyncCommitObserver sco = null;
+ try {
+ sco = new SyncCommitObserver();
+ commitAsync(sco);
+ sco.waitForCommit();
+ } finally {
+ updates.clear();
+ weakNotification = null;
+ observedColumns.clear();
--- End diff --
No worries @keith-turner , thanks for looking at it. Removed observedColumns.clear() allowed the ITs to pass for me locally. We will see what travis has to say. Thanks for opening that other ticket as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #832: closes #809 - cleanup buffered data
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/832#discussion_r115500227
--- Diff: modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java ---
@@ -596,9 +597,14 @@ public CommitData createCommitData() {
@Override
public synchronized void commit() throws CommitException {
- SyncCommitObserver sco = new SyncCommitObserver();
- commitAsync(sco);
- sco.waitForCommit();
+ SyncCommitObserver sco = null;
+ try {
+ sco = new SyncCommitObserver();
+ commitAsync(sco);
+ sco.waitForCommit();
+ } finally {
+ sco = null;
--- End diff --
I wasn't sure if this was necessary, so looked into it and found this [SOQ](http://stackoverflow.com/questions/473685/does-it-help-gc-to-null-local-variables-in-java).
For this issue I think nulling `updates`, `weakNotifications`, `observedColumns`, and `columnsRead` would be good. These can be found starting at [TransactionImpl line 109](https://github.com/apache/incubator-fluo/blob/a1c869ca508a020d028f8942225100523d5379a2/modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java#L109).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #832: closes #809 - cleanup buffered data
Posted by mjwall <gi...@git.apache.org>.
Github user mjwall commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/832#discussion_r115542684
--- Diff: modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java ---
@@ -596,9 +597,14 @@ public CommitData createCommitData() {
@Override
public synchronized void commit() throws CommitException {
- SyncCommitObserver sco = new SyncCommitObserver();
- commitAsync(sco);
- sco.waitForCommit();
+ SyncCommitObserver sco = null;
+ try {
+ sco = new SyncCommitObserver();
+ commitAsync(sco);
+ sco.waitForCommit();
+ } finally {
+ sco = null;
--- End diff --
Thanks for the article. Of those you suggest, I can only null the weakNotifications. The rest are final, but I can clear them since they are collections. I'll push a change for review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #832: closes #809 - cleanup buffered data
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/832#discussion_r115501042
--- Diff: modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java ---
@@ -27,12 +30,6 @@
import java.util.Objects;
import java.util.Set;
-import com.google.common.annotations.VisibleForTesting;
--- End diff --
The build is failing because checkstyle doesn't like this import order. We have instructions for ordering imports in Eclipes and IntelliJ at :
http://fluo.apache.org/how-to-contribute/#coding-guidelines
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo issue #832: closes #809 - cleanup buffered data
Posted by mjwall <gi...@git.apache.org>.
Github user mjwall commented on the issue:
https://github.com/apache/incubator-fluo/pull/832
mvn test ran fine locally, trying mvn verify to see if I get the same errors as travis
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #832: closes #809 - cleanup buffered data
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-fluo/pull/832
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #832: closes #809 - cleanup buffered data
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/832#discussion_r115613754
--- Diff: modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java ---
@@ -596,9 +596,17 @@ public CommitData createCommitData() {
@Override
public synchronized void commit() throws CommitException {
- SyncCommitObserver sco = new SyncCommitObserver();
- commitAsync(sco);
- sco.waitForCommit();
+ SyncCommitObserver sco = null;
+ try {
+ sco = new SyncCommitObserver();
+ commitAsync(sco);
+ sco.waitForCommit();
+ } finally {
+ updates.clear();
+ weakNotification = null;
+ observedColumns.clear();
--- End diff --
@mjwall I found the bug. Its my fault, I suggested clearing `observedColumns` and that is a bad thing to do. `observedColumns` is shared between transactions. It should be an immutable set, I'll open an issue for that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #832: closes #809 - cleanup buffered data
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/832#discussion_r115549403
--- Diff: modules/core/src/main/java/org/apache/fluo/core/impl/TransactionImpl.java ---
@@ -596,9 +597,14 @@ public CommitData createCommitData() {
@Override
public synchronized void commit() throws CommitException {
- SyncCommitObserver sco = new SyncCommitObserver();
- commitAsync(sco);
- sco.waitForCommit();
+ SyncCommitObserver sco = null;
+ try {
+ sco = new SyncCommitObserver();
+ commitAsync(sco);
+ sco.waitForCommit();
+ } finally {
+ sco = null;
--- End diff --
Oh, I didn't notice they were final. Interesting, clear has a cost it loops internally setting stuff to null. I suppose this is ok? Maybe its more little more work here and then less work later for the GC, not sure.
I thought about removing final, but final is nice for ensuring correctness. So I am not inclined to do that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---