Fixed the OOM issue, still have to write tests, release notes, and de… #1678

gmaretic · 2017-03-06T00:38:05Z

…cide how to deal with rows that are too wide

Goals (and why):

Fix the DbKvs sweep OOM issue that arises from the combination of wide rows and large row batch size.

Implementation Description (bullets):

The OOM was happening because the previous implementation created a large multimap before doing the paging. Now we limit the size of the multimap to a predefined maximum (chosen 1M arbitrarily).

Concerns (what feedback would you like?):

Have to decide what to do with rows that are wider than the current GET_RANGE_OF_TS_MAX_BATCH. Current solution is to skip them; alternatively can just process the first GET_RANGE_OF_TS_MAX_BATCH columns. Cannot figure out how to do two pages on the same row as the second set of timestamps seems to just be ignored.

Where should we start reviewing?:

getTimestampsByCell in DbKvs

Priority (whenever / two weeks / yesterday):

Couple of days, as I still have a bunch of things to do before it is ready.

…cide how to deal with rows that are too wide

codecov-io · 2017-03-06T01:00:10Z

Codecov Report

Merging #1678 into develop will increase coverage by 0.01%.
The diff coverage is 75.66%.

@@              Coverage Diff              @@
##             develop    #1678      +/-   ##
=============================================
+ Coverage       59.9%   59.92%   +0.01%     
+ Complexity      4740     4352     -388     
=============================================
  Files            732      735       +3     
  Lines          36141    36276     +135     
  Branches        3953     3967      +14     
=============================================
+ Hits           21651    21738      +87     
- Misses         12984    13029      +45     
- Partials        1506     1509       +3

Impacted Files	Coverage Δ	Complexity Δ
...lasdb/keyvalue/impl/SweepStatsKeyValueService.java	`37.71% <ø> (ø)`	`13 <0> (ø)`	❌
...yvalue/dbkvs/impl/ConnectionManagerAwareDbKvs.java	`92% <ø> (ø)`	`5 <0> (ø)`	❌
...om/palantir/atlasdb/keyvalue/dbkvs/impl/Token.java	`100% <100%> (ø)`	`2 <2> (?)`
...a/com/palantir/atlasdb/sweep/CountingIterator.java	`100% <100%> (ø)`	`3 <1> (-2)`	❌
...om/palantir/atlasdb/sweep/AbstractSweeperTest.java	`88.88% <5.4%> (-8.02%)`	`0 <0> (-75)`
...om/palantir/atlasdb/sweep/SweepTaskRunnerImpl.java	`89.16% <80%> (-0.58%)`	`34 <1> (+1)`
...om/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java	`84.06% <86.84%> (+0.42%)`	`127 <10> (-2)`	❌
...tir/atlasdb/sweep/EquivalenceCountingIterator.java	`90% <90%> (ø)`	`8 <8> (?)`
...antir/atlasdb/keyvalue/dbkvs/impl/DbReadTable.java	`68.96% <93.33%> (+4.1%)`	`17 <1> (+1)`	✅
...ue/dbkvs/impl/TimestampsByCellResultWithToken.java	`97.14% <97.14%> (ø)`	`26 <26> (?)`
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8239e7f...3be2d49. Read the comment docs.

gsheasby

Still need to look at DbReadTable and below.

gsheasby · 2017-03-06T10:56:40Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

@@ -114,6 +114,8 @@
 public class DbKvs extends AbstractKeyValueService {
    private static final Logger log = LoggerFactory.getLogger(DbKvs.class);

+    private static final long GET_RANGE_OF_TS_MAX_BATCH = 1000000L;


nit: underscores to ease readability.

Also, it appears that this is hard-coded and can't be changed. Would be nice to add support for custom batch sizes.

gsheasby · 2017-03-06T12:53:03Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

        try (ClosableIterator<AgnosticLightResultRow> rangeResults = table.getRange(range, timestamp, maxRows)) {
            while (rows.size() < maxRows && rangeResults.hasNext()) {
-                byte[] rowName = rangeResults.next().getBytes("row_name");
+                byte[] rowName = rangeResults.next().getBlob("row_name");


Why was this changed?

getBytes was deprecated, replaced with getBlob which looks like it returns empty arrays rather than null if the column doesn't exist.

see comments below, this may have some perf side effects, would be interested in before/after numbers of this change alone.

gsheasby · 2017-03-06T12:57:17Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

-            nextRow = RangeRequests.getNextStartRow(range.isReverse(), lastRow);
-            mayHaveMoreResults = rows.size() == maxRows;
+        byte[] nextRow = results.currentRow;
+        boolean mayHaveMoreResults = !results.finishedRange;


I would like to see this logic broken out into a place where it's unit testable, as it's quite complex.

gsheasby · 2017-03-06T12:59:56Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

+            if (!Arrays.equals(currentRow, cellResult.getBlob("row_name"))) {
+                currentRow = cellResult.getBlob("row_name");
+                storeRow(rowBuffer);
+                rowBuffer.clear();


It's very weird to have such a side-effect here. I can see why you'd want it - but maybe we should have this method return a boolean stating whether we stored the row, or alternatively split into methods rowIsComplete and storeRow(cellResult, rowBuffer).

Or maybe the abstraction isn't quite right and we need a stateful object to cover the algorithm in getTimestampsByCell, to enforce the "store a row and then clear the buffer" requirement.

schlosna · 2017-03-06T14:04:23Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

+            while (iterator.hasNext() && result.size() + rowBuffer.size() < batchSize) {
+                AgnosticLightResultRow cellResult = iterator.next();
+                result.storePreviousRowIfComplete(cellResult, rowBuffer);
+                Cell cell = Cell.create(cellResult.getBlob("row_name"), cellResult.getBlob("col_name"));


why are we using getBlob for columns that are not BLOB? We should run some perf tests comparing these for current state on Postgres & Oracle, but from a while back using cellResult.getBytes("row_name") was faster & lower overhead than getBlob.

That's a good point, I'll look into getting the numbers before this gets a green light. I only changed it in the first place because getBytes specifically says it's deprecated and getBlob should be used instead.

schlosna · 2017-03-06T14:06:32Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

+        }
+
+        void storePreviousRowIfComplete(AgnosticLightResultRow cellResult, SetMultimap<Cell, Long> rowBuffer) {
+            if (!Arrays.equals(currentRow, cellResult.getBlob("row_name"))) {


see above comment about getBlob vs. getBytes.

Should also pull the row name out to a local to avoid reading twice:

byte[] rowName = cellResult.getBytes("row_name"); if (!Arrays.equals(currentRow, rowName)) { currentRow = rowName;

schlosna · 2017-03-06T14:09:17Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

@@ -1313,4 +1329,34 @@ private static Integer getMaxRowsFromBatchHint(@Nullable Integer batchHint) {
    private interface ReadWriteTask<T> {
        T run(DbReadTable readTable, DbWriteTable writeTable);
    }
+
+    static class ResultWithToken {
+        SetMultimap<Cell, Long> results;


nit: final

schlosna · 2017-03-06T14:12:44Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbReadTable.java

+    }
+
+    public ClosableIterator<AgnosticLightResultRow> getAllRows(
+            Iterable<byte[]> rows, ColumnSelection columns, long ts, boolean includeValues, Boolean reverseRows) {


taking a Boolean arg here feels weird, why not make this a primitive or enum (ASCENDING, DESCENDING, UNDEFINED) for clarity as caller should expressly know desired order?

schlosna · 2017-03-06T14:15:21Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

        try (ClosableIterator<AgnosticLightResultRow> rangeResults = table.getRange(range, timestamp, maxRows)) {
            while (rows.size() < maxRows && rangeResults.hasNext()) {
-                byte[] rowName = rangeResults.next().getBytes("row_name");
+                byte[] rowName = rangeResults.next().getBlob("row_name");


see comments below, this may have some perf side effects, would be interested in before/after numbers of this change alone.

jeremyk-91 · 2017-03-06T10:11:59Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

-                Cell cell = Cell.create(row.getBytes("row_name"), row.getBytes("col_name"));
-                long ts = row.getLong("ts");
-                results.put(cell, ts);
+    private ResultWithToken getTimestampsByCell(


I realise this is probably not in the scope you were going for, but I think Javadoc comments / multiline comments on these methods would be really nice for understanding.

That is actually very helpful for the future!

jeremyk-91 · 2017-03-06T14:04:38Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

        try (ClosableIterator<AgnosticLightResultRow> rangeResults = table.getRange(range, timestamp, maxRows)) {
            while (rows.size() < maxRows && rangeResults.hasNext()) {
-                byte[] rowName = rangeResults.next().getBytes("row_name");
+                byte[] rowName = rangeResults.next().getBlob("row_name");


getBytes was deprecated, replaced with getBlob which looks like it returns empty arrays rather than null if the column doesn't exist.

jeremyk-91 · 2017-03-06T14:08:27Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

        try (ClosableIterator<AgnosticLightResultRow> rangeResults = table.getRange(range, timestamp, maxRows)) {
            while (rows.size() < maxRows && rangeResults.hasNext()) {
-                byte[] rowName = rangeResults.next().getBytes("row_name");
+                byte[] rowName = rangeResults.next().getBlob("row_name");
                if (rowName != null) {


Shouldn't need this check (I think) since getBlob returns nonnull

jeremyk-91 · 2017-03-06T14:16:28Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbReadTable.java

        }
    }

+    private FullQuery addOrdering(Optional<Boolean> reverseRows, FullQuery query) {


Could we use Optional above in getAllRows or just a Boolean object here? I'd prefer for both to be consistent, if that's feasible.

Perhaps even better: an enum for ordering - undefined, descending, or ascending.

jeremyk-91 · 2017-03-06T14:18:27Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

+                .getAllRows(rows, columns, timestamp, false, isReverse)) {
+            ResultWithToken result = new ResultWithToken();
+            SetMultimap<Cell, Long> rowBuffer = HashMultimap.create();
+            while (iterator.hasNext() && result.size() + rowBuffer.size() < batchSize) {


Probably worth a note here that size() is actually constant time; I was a bit concerned about perf here, but looked it up and seems fine (AbstractMapBasedMultimap keeps track of its total size)

gsheasby · 2017-03-06T14:18:11Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbReadTable.java

@@ -62,16 +64,30 @@ public DbReadTable(ConnectionSupplier conns, DbQueryFactory queryFactory) {

    public ClosableIterator<AgnosticLightResultRow> getAllRows(
            Iterable<byte[]> rows, ColumnSelection columns, long ts, boolean includeValues) {
+        return getAllRows(rows, columns, ts, includeValues, null);


Passing null here is really ugly - as DavidS said, I'd prefer some kind of enum here.

gsheasby · 2017-03-06T14:21:08Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbReadTable.java

        }
    }

+    private FullQuery addOrdering(Optional<Boolean> reverseRows, FullQuery query) {
+        String ordering = reverseRows
+                .map(reverse -> " ORDER BY m.row_name" + (reverse ? " DESC" : " ASC") + ", m.col_name, m.ts")


nit: the space should probably go in the " ORDER BY m.row_name" substring.

… some tests. Still have to add more tests, do some refactoring, add javadocs, and release notes.

gmaretic · 2017-03-07T01:07:07Z

Thanks for the reviews, I have addressed most of it, and added a fix for the wide rows case. There is no hurry to review, as I still have several things to do, so some refactors are expected as well!

gsheasby · 2017-03-08T14:08:50Z

...-dbkvs-tests/src/test/java/com/palantir/atlasdb/keyvalue/dbkvs/DbkvsPostgresSweeperTest.java

+        assertThat(results.getNextStartRow().isPresent()).isFalse();
+        assertThat(results.getCellsExamined()).isEqualTo(9);
+        assertThat(results.getCellsDeleted()).isEqualTo(240);
+        resetGetRangeOfTsMaxBatch();


resetGetRangeOfTsMaxBatch could be done in an @After method.

gsheasby · 2017-03-08T14:09:39Z

...-dbkvs-tests/src/test/java/com/palantir/atlasdb/keyvalue/dbkvs/DbkvsPostgresSweeperTest.java

+        resetGetRangeOfTsMaxBatch();
+    }
+
+    private void setGetRangeOfTsMaxBatch(long value) {


Duplicate code (here until the end of the file). Would be nice to have this code in one place only, e.g. something like DbKvsTestUtils.

Makes sense, refactored

gsheasby · 2017-03-08T14:10:39Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

@@ -113,8 +114,12 @@

 public class DbKvs extends AbstractKeyValueService {
    private static final Logger log = LoggerFactory.getLogger(DbKvs.class);
+    public static final String ROW = "row_name";


Private and public constants should be separate.

gsheasby · 2017-03-08T14:23:30Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

                Long overflowId = hasOverflow ? row.getLongObject("overflow") : null;
                if (overflowId == null) {
-                    Value value = Value.create(row.getBytes("val"), row.getLong("ts"));
+                    Value value = Value.create(row.getBytes("val"), row.getLong(TIMESTAMP));


did you consider extracting "val", too?

gsheasby · 2017-03-08T14:24:47Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java


-    private static final long GET_RANGE_OF_TS_MAX_BATCH = 1000000L;
+    private long getRangeOfTsMaxBatch = INITIAL_GET_RANGE_OF_TS_BATCH;


perhaps call this maxRangeOfTimestampsBatchSize? And "INITIAL" -> "DEFAULT".

gsheasby · 2017-03-08T14:41:09Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

+         *  3. In the same column, with higher or equal timestamp (to repeat the last timestamp for sweep)
+         */
+        void moveForward(Token oldToken) {
+            boolean forwarding = oldToken.shouldForward();


Perhaps rename forwarding to movingForward and shouldForward to shouldMoveForward?

Or skipping / shouldSkip / shouldFinishSkipping.

gsheasby · 2017-03-08T14:46:51Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

-            }
+                .getAllRows(rows, columns, timestamp, false, DbReadTable.Order.fromBoolean(isReverse))) {
+            ResultWithToken result = ResultWithToken.create(iterator);
+            result.moveForward(token);


It looks like ResultWithToken must be used in this way (calling create, moveForward, getBatchOfTimestamps, and checkNextEntryAndCreateToken, in that order), and the object doesn't really make sense until all that has been done.

We could enforce this temporal dependency by moving the other three methods inside create, which would then take the iterator, the previous token, and the batch size.

Agree, makes much more sense

gsheasby · 2017-03-08T14:52:40Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/EquivalenceCountingIterator.java

+    @Override
+    public boolean hasNext() {
+        if (delegate.hasNext()) {
+            if (limit >= totalItems + returnOneIfNextRowIsNew()) {


How about an int-returning method getTotalItems() that looks like:

if (nextRowIsNew()) { return totalItems + 1; } else { return totalItems; }

This was so obvious 🦆

gsheasby · 2017-03-08T14:53:11Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/EquivalenceCountingIterator.java

+
+    @Override
+    public void remove() {
+    // not implemented


Throw NotImplementedException?

gsheasby · 2017-03-08T14:56:07Z

docs/source/release_notes/release-notes.rst

@@ -40,6 +40,10 @@ develop
    *    - Type
         - Change

+    *    - |fixed|
+         - Fixed DbKvs sweep OOM issue `#982 <https://github.com/palantir/atlasdb/issues/982>`__ caused by very wide rows. Now uses cell batch size that


Incomplete release note!

gsheasby

Just a few nits left, mostly around TimestampsByCellResultWithToken.

gsheasby · 2017-03-09T10:40:48Z

.../src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/TimestampsByCellResultWithToken.java

+        boolean skipping = oldToken.shouldSkip();
+        while (skipping && iterator.hasNext()) {
+            AgnosticLightResultRow nextResult = iterator.peek();
+            if (finishedForwarding(oldToken, nextResult)) {


Oops, you missed a rename: finishedForwarding -> finishedSkipping

gsheasby · 2017-03-09T10:42:56Z

docs/source/release_notes/release-notes.rst

@@ -44,6 +44,10 @@ develop
         - Fixed DbKvs sweep OOM issue `#982 <https://github.com/palantir/atlasdb/issues/982>`__ caused by very wide rows. Now uses cell batch size that


Release note still incomplete.

gsheasby · 2017-03-09T10:58:51Z

atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/DbKvs.java

-        List<RowResult<Set<Long>>> finalResults = new ArrayList<>();
-        cellsByRow.entrySet().forEach(entry -> finalResults.add(RowResult.create(entry.getKey(), entry.getValue())));
+        List<RowResult<Set<Long>>> finalResults = cellsByRow.entrySet().stream()
+                .map(entry -> RowResult.create(entry.getKey(), entry.getValue())).collect(Collectors.toList());


super-nit: should have .map and .collect on separate lines.

gsheasby · 2017-03-09T11:00:31Z

.../src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/TimestampsByCellResultWithToken.java

+import com.palantir.nexus.db.sql.AgnosticLightResultRow;
+
+final class TimestampsByCellResultWithToken {
+    private static final String ROW = "row_name";


These are duplicates of DbKvs.ROW etc.

gsheasby · 2017-03-09T11:01:45Z

.../src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/TimestampsByCellResultWithToken.java

+     *  2. In a greater column
+     *  3. In the same column, with higher or equal timestamp (to repeat the last timestamp for sweep)
+     */
+    TimestampsByCellResultWithToken moveForward(Token oldToken) {


Can these methods (all except create) now be private?

gsheasby · 2017-03-09T11:05:09Z

.../src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/impl/TimestampsByCellResultWithToken.java

+    private PeekingIterator<AgnosticLightResultRow> iterator;
+
+    final SetMultimap<Cell, Long> entries;
+    boolean mayHaveMoreResults = false;


Should probably expose getters for these and make them private, to avoid modification.

gsheasby · 2017-03-09T11:07:06Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/EquivalenceCountingIterator.java

    }

    @Override
    public void remove() {
-    // not implemented
+        throw new RuntimeException("Method remove() is not implemented.");


nit: UnsupportedOperationException (java.lang) is more descriptive.

gsheasby · 2017-03-09T15:41:12Z

...db-dbkvs-tests/src/test/java/com/palantir/atlasdb/keyvalue/dbkvs/DbkvsPostgresTestSuite.java

@@ -38,10 +38,10 @@

 @RunWith(Suite.class)
 @SuiteClasses({
-        DbkvsPostgresKeyValueServiceTest.class,
-        DbkvsPostgresSerializableTransactionTest.class,
+//        DbkvsPostgresKeyValueServiceTest.class,


Make sure this is reverted before we merge!

gsheasby · 2017-03-09T15:43:48Z

atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/sweep/AbstractSweeperTest.java

@@ -576,6 +576,27 @@ public void testOom() {
        Assert.assertEquals(10000L, results.getCellsDeleted());
    }

+    @Test
+    public void wideRowTest() {


How long does this test take to run?

gsheasby

Almost there. Last remaining things before we merge:

one explanatory comment
add ADR

gsheasby · 2017-03-10T10:16:31Z

...ests/src/test/java/com/palantir/atlasdb/keyvalue/dbkvs/AbstractDbKvsKeyValueServiceTest.java

+                62)) {
+            EquivalenceCountingIterator<RowResult<Set<Long>>> iterator =
+                    new EquivalenceCountingIterator<>(rowResults, 10, SweepTaskRunnerImpl.sameRowEquivalence());
+            assertRowColumnsTimestamps(iterator.next(), 1, ImmutableSet.of(1, 2, 3, 4),


I'm concerned that this call is hard to read. Might be good to add a comment that the arguments are: result, expected row key, expected columns, expected timestamps.

gsheasby

LGTM!

gsheasby · 2017-03-10T13:21:27Z

Looks like you have checkstyle errors, and need to update the dependency lock.

Fixed the OOM issue, still have to write tests, release notes, and de…

bd6d94b

…cide how to deal with rows that are too wide

gmaretic added component: sweep do not merge labels Mar 6, 2017

gmaretic requested review from jboreiko, jeremyk-91 and gsheasby March 6, 2017 00:38

gsheasby suggested changes Mar 6, 2017

View reviewed changes

schlosna reviewed Mar 6, 2017

View reviewed changes

jeremyk-91 reviewed Mar 6, 2017

View reviewed changes

gsheasby suggested changes Mar 6, 2017

View reviewed changes

Addressed some of the comments, now works with super-wide rows, added…

a9fd37f

… some tests. Still have to add more tests, do some refactoring, add javadocs, and release notes.

gmaretic and others added 8 commits March 7, 2017 17:39

Refactored and renamed iterator, wrote tests for it

67dcc23

Finished tests and refactored

469f308

Added release notes, minor style fix

7653592

Nits

d0aa4c5

Fixed OOM test, added a note about it

c2bf601

Moved OOM test

36ba732

Abstracted new DbKvs tests to also be used by Oracle

f49c9cc

Merge branch 'develop' into fix/dbkvs-oom

2c27af6

gsheasby suggested changes Mar 8, 2017

View reviewed changes

Addressed comments, some more minor refactoring

1893bf5

gsheasby suggested changes Mar 9, 2017

View reviewed changes

Addressed comments

f935587

gsheasby reviewed Mar 9, 2017

View reviewed changes

gmaretic added 3 commits March 9, 2017 18:49

Implemented the discussed soution

72184a9

Added another paging test

f5ca40b

Updated javadocs, improved test for paging

2e310a4

gsheasby reviewed Mar 10, 2017

View reviewed changes

gmaretic added 4 commits March 10, 2017 11:02

Moved abstract test classes so that Oracle can use them

0219285

Added javadoc for the paging test

b1b89a4

Fixing merge conflicts

30a688d

Added ADR, merged with develop

ca3b969

gsheasby approved these changes Mar 10, 2017

View reviewed changes

Fixed javadoc, updated locks

34cba34

gmaretic removed the do not merge label Mar 10, 2017

Fixed release notes

3be2d49

gsheasby merged commit 643a5e1 into develop Mar 10, 2017

gsheasby deleted the fix/dbkvs-oom branch March 10, 2017 15:37

gsheasby mentioned this pull request Mar 10, 2017

Sweep shouldn't OOM when iterating through RowResults #982

Closed


		private static final long GET_RANGE_OF_TS_MAX_BATCH = 1000000L;
		private long getRangeOfTsMaxBatch = INITIAL_GET_RANGE_OF_TS_BATCH;

		@@ -44,6 +44,10 @@ develop
		- Fixed DbKvs sweep OOM issue `#982 <https://github.com/palantir/atlasdb/issues/982>`__ caused by very wide rows. Now uses cell batch size that

Fixed the OOM issue, still have to write tests, release notes, and de… #1678

Fixed the OOM issue, still have to write tests, release notes, and de… #1678

Conversation

gmaretic commented Mar 6, 2017

codecov-io commented Mar 6, 2017 • edited

Codecov Report

gsheasby left a 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

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

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

jeremyk-91 Mar 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaretic commented Mar 7, 2017

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

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

gsheasby left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsheasby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsheasby left a comment

Choose a reason for hiding this comment

gsheasby commented Mar 10, 2017

codecov-io commented Mar 6, 2017 •

edited

jeremyk-91 Mar 6, 2017 •

edited