text:
Count number of upsert table is incorrect when a new segment is created

We found an issue on pinot release-1.0.0, when a new segment is created, select count(*) on the table may result in a much smaller value than the actual value.
In my testing, I firstly ingested 1 million data to pinot with all unique ids ranged from 0 to 1000000, then repeat the process to send data with same ids from 0 to 1000000 but with different column values. At the same time, another process execute count(*) query every 100ms to record count number.
Here is the count number result, we can see many anomalous numbers, and their timing is exactly when new segments were created.

I found in broker logs, most issues happened between IDEAL_STATE change and EXTERNAL_VIEW change. So I suspect the problem occurs between these two state changes. Do you have any idea?
023/11/03 17:50:06.769 INFO [QueryLogger] [jersey-server-managed-async-executor-7] requestId=621783916000003375,table=issuerrisk,timeMs=1,docs=1000000/1869645,entries=0/0,segments(queried/processed/matched/consumingQueried/consumingProcessed/consumingMatched/unavailable):8/8/4/1/0/0/0,consumingFreshnessTimeMs=1699005001041,servers=1/1,groupLimitReached=false,brokerReduceTimeMs=0,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs);192.168.31.226_R=0,1,264,0,0,offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,clientIp=unknown,query=select count(*) as count from issuerrisk
2023/11/03 17:50:06.771 INFO [CallbackHandler] [ZkClient-EventThread-73-localhost:2181] 73 START: CallbackHandler 0, INVOKE /PinotCluster/IDEALSTATES listener: org.apache.pinot.broker.broker.helix.ClusterChangeMediator@465fefa9 type: CALLBACK
2023/11/03 17:50:06.771 INFO [ClusterChangeMediator] [ZkClient-EventThread-73-localhost:2181] Enqueueing IDEAL_STATE change
2023/11/03 17:50:06.771 INFO [CallbackHandler] [ZkClient-EventThread-73-localhost:2181] 73 END:INVOKE CallbackHandler 0, /PinotCluster/IDEALSTATES listener: org.apache.pinot.broker.broker.helix.ClusterChangeMediator@465fefa9 type: CALLBACK Took: 0ms
2023/11/03 17:50:06.771 INFO [ClusterChangeMediator] [ClusterChangeHandlingThread] Start processing IDEAL_STATE change
2023/11/03 17:50:06.771 INFO [BrokerRoutingManager] [ClusterChangeHandlingThread] Processing segment assignment change
2023/11/03 17:50:06.774 INFO [BaseInstanceSelector] [ClusterChangeHandlingThread] Got 1 new segments: {issuerrisk__0__8__20231103T0950Z=1699005006774} for table: issuerrisk_REALTIME by processing existing states, current time: 1699005006774
2023/11/03 17:50:06.774 INFO [BrokerRoutingManager] [ClusterChangeHandlingThread] Processed segment assignment change in 3ms (fetch ideal state and external view stats for 1 tables: 0ms, update routing entry for 1 tables ([issuerrisk_REALTIME]): 3ms)
2023/11/03 17:50:06.774 INFO [ClusterChangeMediator] [ClusterChangeHandlingThread] Finish handling IDEAL_STATE change for handler: BrokerRoutingManager in 3ms
2023/11/03 17:50:06.774 INFO [ClusterChangeMediator] [ClusterChangeHandlingThread] Finish processing IDEAL_STATE change in 3ms
2023/11/03 17:50:06.876 INFO [QueryLogger] [jersey-server-managed-async-executor-7] requestId=621783916000003376,table=issuerrisk,timeMs=0,docs=1000000/1869645,entries=0/0,segments(queried/processed/matched/consumingQueried/consumingProcessed/consumingMatched/unavailable):8/8/4/1/0/0/0,consumingFreshnessTimeMs=1699005001041,servers=1/1,groupLimitReached=false,brokerReduceTimeMs=0,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs);192.168.31.226_R=0,1,264,0,0,offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,clientIp=unknown,query=select count(*) as count from issuerrisk
2023/11/03 17:50:06.998 INFO [QueryLogger] [jersey-server-managed-async-executor-7] requestId=621783916000003377,table=issuerrisk,timeMs=1,docs=740101/1869645,entries=0/0,segments(queried/processed/matched/consumingQueried/consumingProcessed/consumingMatched/unavailable):8/8/4/0/0/0/0,consumingFreshnessTimeMs=1699005002130,servers=1/1,groupLimitReached=false,brokerReduceTimeMs=0,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs);192.168.31.226_R=0,1,256,0,1,offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,clientIp=unknown,query=select count(*) as count from issuerrisk
2023/11/03 17:50:07.094 INFO [CallbackHandler] [ZkClient-EventThread-73-localhost:2181] 73 START: CallbackHandler 1, INVOKE /PinotCluster/EXTERNALVIEW listener: org.apache.pinot.broker.broker.helix.ClusterChangeMediator@465fefa9 type: CALLBACK
2023/11/03 17:50:07.094 INFO [ClusterChangeMediator] [ZkClient-EventThread-73-localhost:2181] Enqueueing EXTERNAL_VIEW change
2023/11/03 17:50:07.094 INFO [CallbackHandler] [ZkClient-EventThread-73-localhost:2181] 73 END:INVOKE CallbackHandler 1, /PinotCluster/EXTERNALVIEW listener: org.apache.pinot.broker.broker.helix.ClusterChangeMediator@465fefa9 type: CALLBACK Took: 0ms
2023/11/03 17:50:07.094 INFO [ClusterChangeMediator] [ClusterChangeHandlingThread] Start processing EXTERNAL_VIEW change
2023/11/03 17:50:07.094 INFO [BrokerRoutingManager] [ClusterChangeHandlingThread] Processing segment assignment change
2023/11/03 17:50:07.096 INFO [BaseInstanceSelector] [ClusterChangeHandlingThread] Got 0 new segments: {} for table: issuerrisk_REALTIME by processing existing states, current time: 1699005007096
2023/11/03 17:50:07.096 INFO [BrokerRoutingManager] [ClusterChangeHandlingThread] Processed segment assignment change in 2ms (fetch ideal state and external view stats for 1 tables: 1ms, update routing entry for 1 tables ([issuerrisk_REALTIME]): 1ms)
2023/11/03 17:50:07.096 INFO [ClusterChangeMediator] [ClusterChangeHandlingThread] Finish handling EXTERNAL_VIEW change for handler: BrokerRoutingManager in 2ms
2023/11/03 17:50:07.096 INFO [HelixExternalViewBasedQueryQuotaManager] [ClusterChangeHandlingThread] Start processing qps quota change.
2023/11/03 17:50:07.096 INFO [HelixExternalViewBasedQueryQuotaManager] [ClusterChangeHandlingThread] No qps quota change: external view for broker resource remains the same.
2023/11/03 17:50:07.096 INFO [ClusterChangeMediator] [ClusterChangeHandlingThread] Finish handling EXTERNAL_VIEW change for handler: HelixExternalViewBasedQueryQuotaManager in 0ms
2023/11/03 17:50:07.096 INFO [ClusterChangeMediator] [ClusterChangeHandlingThread] Finish processing EXTERNAL_VIEW change in 2ms
2023/11/03 17:50:07.109 INFO [QueryLogger] [jersey-server-managed-async-executor-7] requestId=621783916000003378,table=issuerrisk,timeMs=2,docs=875721/1873069,entries=0/0,segments(queried/processed/matched/consumingQueried/consumingProcessed/consumingMatched/unavailable):9/9/5/1/0/0/0,consumingFreshnessTimeMs=1699005007107,servers=1/1,groupLimitReached=false,brokerReduceTimeMs=0,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs);192.168.31.226_R=0,2,264,0,0,offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,clientIp=unknown,query=select count(*) as count from issuerrisk
2023/11/03 17:50:07.218 INFO [QueryLogger] [jersey-server-managed-async-executor-7] requestId=621783916000003379,table=issuerrisk,timeMs=1,docs=999998/1879272,entries=0/0,segments(queried/processed/matched/consumingQueried/consumingProcessed/consumingMatched/unavailable):9/9/5/1/0/0/0,consumingFreshnessTimeMs=1699005007217,servers=1/1,groupLimitReached=false,brokerReduceTimeMs=0,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs);192.168.31.226_R=0,1,264,0,0,offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,clientIp=unknown,query=select count(*) as count from issuerrisk
2023/11/03 17:50:07.304 INFO [CallbackHandler] [ZkClient-EventThread-73-localhost:2181] 73 START: CallbackHandler 1, INVOKE /PinotCluster/EXTERNALVIEW listener: org.apache.pinot.broker.broker.helix.ClusterChangeMediator@465fefa9 type: CALLBACK
2023/11/03 17:50:07.304 INFO [ClusterChangeMediator] [ZkClient-EventThread-73-localhost:2181] Enqueueing EXTERNAL_VIEW change
2023/11/03 17:50:07.304 INFO [CallbackHandler] [ZkClient-EventThread-73-localhost:2181] 73 END:INVOKE CallbackHandler 1, /PinotCluster/EXTERNALVIEW listener: org.apache.pinot.broker.broker.helix.ClusterChangeMediator@465fefa9 type: CALLBACK Took: 0ms
2023/11/03 17:50:07.304 INFO [ClusterChangeMediator] [ClusterChangeHandlingThread] Start processing EXTERNAL_VIEW change
2023/11/03 17:50:07.304 INFO [BrokerRoutingManager] [ClusterChangeHandlingThread] Processing segment assignment change
2023/11/03 17:50:07.309 INFO [BaseInstanceSelector] [ClusterChangeHandlingThread] Got 0 new segments: {} for table: issuerrisk_REALTIME by processing existing states, current time: 1699005007309
2023/11/03 17:50:07.309 INFO [BrokerRoutingManager] [ClusterChangeHandlingThread] Processed segment assignment change in 5ms (fetch ideal state and external view stats for 1 tables: 2ms, update routing entry for 1 tables ([issuerrisk_REALTIME]): 3ms)
2023/11/03 17:50:07.309 INFO [ClusterChangeMediator] [ClusterChangeHandlingThread] Finish handling EXTERNAL_VIEW change for handler: BrokerRoutingManager in 5ms
2023/11/03 17:50:07.309 INFO [HelixExternalViewBasedQueryQuotaManager] [ClusterChangeHandlingThread] Start processing qps quota change.
2023/11/03 17:50:07.309 INFO [HelixExternalViewBasedQueryQuotaManager] [ClusterChangeHandlingThread] No qps quota change: external view for broker resource remains the same.
2023/11/03 17:50:07.309 INFO [ClusterChangeMediator] [ClusterChangeHandlingThread] Finish handling EXTERNAL_VIEW change for handler: HelixExternalViewBasedQueryQuotaManager in 0ms
2023/11/03 17:50:07.309 INFO [ClusterChangeMediator] [ClusterChangeHandlingThread] Finish processing EXTERNAL_VIEW change in 5ms
2023/11/03 17:50:07.326 INFO [QueryLogger] [jersey-server-managed-async-executor-7] requestId=621783916000003380,table=issuerrisk,timeMs=1,docs=999998/1886050,entries=0/0,segments(queried/processed/matched/consumingQueried/consumingProcessed/consumingMatched/unavailable):9/9/5/1/0/0/0,consumingFreshnessTimeMs=1699005007326,servers=1/1,groupLimitReached=false,brokerReduceTimeMs=0,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs);192.168.31.226_R=0,1,264,0,0,offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,clientIp=unknown,query=select count(*) as count from issuerrisk
2023/11/03 17:50:07.434 INFO [QueryLogger] [jersey-server-managed-async-executor-7] requestId=621783916000003381,table=issuerrisk,timeMs=1,docs=999999/1895348,entries=0/0,segments(queried/processed/matched/consumingQueried/consumingProcessed/consumingMatched/unavailable):9/9/5/1/0/0/0,consumingFreshnessTimeMs=1699005007433,servers=1/1,groupLimitReached=false,brokerReduceTimeMs=0,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs);192.168.31.226_R=0,1,264,0,0,offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,clientIp=unknown,query=select count(*) as count from issuerrisk
2023/11/03 17:50:07.545 INFO [QueryLogger] [jersey-server-managed-async-executor-7] requestId=621783916000003382,table=issuerrisk,timeMs=1,docs=999998/1907060,entries=0/0,segments(queried/processed/matched/consumingQueried/consumingProcessed/consumingMatched/unavailable):9/9/5/1/0/0/0,consumingFreshnessTimeMs=1699005007544,servers=1/1,groupLimitReached=false,brokerReduceTimeMs=0,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs);192.168.31.226_R=0,1,264,0,0,offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,clientIp=unknown,query=select count(*) as count from issuerrisk
2023/11/03 17:50:07.652 INFO [QueryLogger] [jersey-server-managed-async-executor-7] requestId=621783916000003383,table=issuerrisk,timeMs=1,docs=1000000/1910088,entries=0/0,segments(queried/processed/matched/consumingQueried/consumingProcessed/consumingMatched/unavailable):9/9/5/1/0/0/0,consumingFreshnessTimeMs=1699005007644,servers=1/1,groupLimitReached=false,brokerReduceTimeMs=0,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs);192.168.31.226_R=0,1,264,0,0,offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,clientIp=unknown,query=select count(*) as count from issuerrisk

Wrong number logs are these two lines.
2023/11/03 17:50:06.998 INFO [QueryLogger] [jersey-server-managed-async-executor-7] requestId=621783916000003377,table=issuerrisk,timeMs=1,docs=740101/1869645,entries=0/0,segments(queried/processed/matched/consumingQueried/consumingProcessed/consumingMatched/unavailable):8/8/4/0/0/0/0,consumingFreshnessTimeMs=1699005002130,servers=1/1,groupLimitReached=false,brokerReduceTimeMs=0,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs);192.168.31.226_R=0,1,256,0,1,offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,clientIp=unknown,query=select count(*) as count from issuerrisk
2023/11/03 17:50:07.109 INFO [QueryLogger] [jersey-server-managed-async-executor-7] requestId=621783916000003378,table=issuerrisk,timeMs=2,docs=875721/1873069,entries=0/0,segments(queried/processed/matched/consumingQueried/consumingProcessed/consumingMatched/unavailable):9/9/5/1/0/0/0,consumingFreshnessTimeMs=1699005007107,servers=1/1,groupLimitReached=false,brokerReduceTimeMs=0,exceptions=0,serverStats=(Server=SubmitDelayMs,ResponseDelayMs,ResponseSize,DeserializationTimeMs,RequestSentDelayMs);192.168.31.226_R=0,2,264,0,0,offlineThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,realtimeThreadCpuTimeNs(total/thread/sysActivity/resSer):0/0/0/0,clientIp=unknown,query=select count(*) as count from issuerrisk

Here is my table definition.
{
    "tableName": "issuerrisk",
    "tableType": "REALTIME",
    "segmentsConfig": {
        "schemaName": "issuerrisk",
        "timeColumnName": "UpdatedTime",
        "allowNullTimeValue": false,
        "replicasPerPartition": "1",
        "completionConfig": {
            "completionMode": "DOWNLOAD"
        }
    },
    "tableIndexConfig": {
        "invertedIndexColumns": [],
        "sortedColumn": [],
        "noDictionaryColumns": [
            "JTD1",
            "JTD2",
            "JTD3",
            "JTD4",
            "JTD5",
            "JTD6",
            "JTD7",
            "JTD8",
            "JTD9",
            "JTD10",
            "Content"
        ],
        "loadMode": "MMAP",
        "nullHandlingEnabled": false
    },
    "ingestionConfig": {
        "streamIngestionConfig": {
            "streamConfigMaps": [
                {
                    "streamType": "kafka",
                    "stream.kafka.consumer.type": "lowlevel",
                    "stream.kafka.topic.name": "issuerrisk",
                    "stream.kafka.decoder.class.name": "org.apache.pinot.plugin.stream.kafka.KafkaJSONMessageDecoder",
                    "stream.kafka.consumer.factory.class.name": "org.apache.pinot.plugin.stream.kafka20.KafkaConsumerFactory",
                    "stream.kafka.broker.list": "localhost:9092",
                    "realtime.segment.flush.threshold.rows": "0",
                    "realtime.segment.flush.threshold.time": "24h",
                    "realtime.segment.flush.threshold.segment.size": "50M",
                    "stream.kafka.consumer.prop.auto.offset.reset": "largest"
                }
            ]
        }
    },
    "tenants": {},
    "metadata": {},
    "upsertConfig": {
        "mode": "FULL",
        "comparisonColumn": "UpdatedTime"
    },
    "routing": {
        "instanceSelectorType": "strictReplicaGroup"
    }
}

Thanks for reporting the issue. We will take a look
cc @klsince @KKcorps

Is the consumption and producer still running even after publishing 1M ids?

Nice test setup. If you can customize it, could you test with a table without using upsert, just to see if this issue is specific to upsert table or not. Thanks!

After publishing 1M ids, producer continued to send 2M upsert data with same ids as first 1M ids.
Producer code like this:
def generate_record(id):
    record = {
        'UID': id,
        'UpdatedTime': get_time(),
        'Content': generate_random_string(CONTENT_LENGTH)
    }
    for i in range(1, 11):
        record[f'JTD{i}'] = generate_random_number()
    return json.dumps(record)

for i in range(3):
    for id in range(1_000_000):
        record = generate_record(id)
        producer.send(TOPIC, key=str(id).encode(), value=record.encode())
        if id % 10000 == 0:
            print(f'Published {i} round, {id} messages')
            producer.flush()

producer.flush()

I tested again without upsert, no this issue. So the issue is only on upsert table.

cool! How about using this query set "skipUpsert"=true; select count(*) ... to run test with the upsert table again, to see if any critical upsert states were not visible to the query when the segment was being commit.
(I'll try to reproduce this on my side for some clues)

is the table config posted above the complete config? There is no segmentPartitionConfig, which defines table partitioning required by upsert tables to place the segments and route queries properly.
How many servers are hosting segments for the table?

That's all table configs. I rely on kafka partition so I don't need segmentPartitionConfig, and in my testing the kafka topic only have one partition. And only one server.

Tested select count with "skipUpsert"=true, no this issue.

I was able to reproduce the issue on my side as well. will debug this further. stay tuned..

While debugging the issue, we found this bug and fix it here: https://github.com/apache/pinot/pull/11964
But even with this fix, I could still see the count(*) queries returned smaller values than expected, when new segments were created. Because there is another issue on query routing as described here: https://github.com/apache/pinot/issues/11965, will address this as next.

@klsince Tested on your fix branch #11978, the result is pretty good, large count inconsistency is fixed.

One more interesting thing is there are still many small differences, some smaller that correct count, some larger than correct count.

hey @jhyao thanks for testing that out on your side. Yes, I also noticed the overcounting issue while testing the new changes. Basically, the realtime data ingestion can continue to update existing segments (like the upsert validDocId bitmaps used to identify which docs are updated in each segment) while the query is processing those segments in parallel (and without a deterministic segment processing order).
The query could overcount or undercount, depending on the order of segment processing and whether new records got ingested and invalidated existing docs. But this is different from the issue to be fixed by PR https://github.com/apache/pinot/pull/11978, because no segments were missed just that they got updated during query execution.

authors: Jackie-Jiang KKcorps jhyao klsince

repo: apache/pinot


text:
[multistage] Support Geometry queries

Support st-* functions in multi-stage engine
(TODO) Add integration test

Codecov Report

Merging #11191 (734ef18) into master (d83d1e8) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #11191      +/-   ##
==========================================
- Coverage    0.11%    0.11%   -0.01%
==========================================
  Files        2219     2219
  Lines      119191   119199       +8
  Branches    18031    18031
==========================================
  Hits          137      137
- Misses     119034   119042       +8
  Partials       20       20

Flag
Coverage Δ

integration1temurin11
?

integration1temurin17
?

integration1temurin20
0.00% <0.00%> (ø)

integration2temurin11
?

integration2temurin20
?

unittests1temurin11
?

unittests1temurin17
0.00% <0.00%> (ø)

unittests1temurin20
0.00% <0.00%> (ø)

unittests2temurin11
0.11% <0.00%> (-0.01%)
:arrow_down:

unittests2temurin17
0.11% <0.00%> (-0.01%)
:arrow_down:

unittests2temurin20
0.11% <0.00%> (-0.01%)
:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed
Coverage Δ

...e/pinot/common/function/TransformFunctionType.java
0.00% <0.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

source: gharchive/pull-request

license_type: permissive

license_source: github-api


text:
Cleanup segment upload logic and allow validation on real-time table

Subtask of #8283

Cleanup the segment upload logic and add more checks on the parameters
Refactor the segment validator to a util class to make it work on both metadata push and real-time table
Refine the return code documentation of the segment upload API

Codecov Report

Merging #8695 (7ce35ef) into master (088da3f) will decrease coverage by 40.87%.
The diff coverage is 54.13%.

@@              Coverage Diff              @@
##             master    #8695       +/-   ##
=============================================
- Coverage     66.16%   25.29%   -40.88%
+ Complexity     4387       45     -4342
=============================================
  Files          1277     1710      +433
  Lines         64430    89493    +25063
  Branches      10014    13290     +3276
=============================================
- Hits          42633    22638    -19995
- Misses        18786    64691    +45905
+ Partials       3011     2164      -847

Flag
Coverage Δ

integration2
25.29% <54.13%> (?)

unittests1
?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

...ntroller/helix/core/PinotHelixResourceManager.java
31.03% <ø> (ø)

.../controller/api/upload/SegmentValidationUtils.java
40.00% <40.00%> (ø)

...ces/PinotSegmentUploadDownloadRestletResource.java
43.80% <55.71%> (ø)

...apache/pinot/controller/api/upload/ZKOperator.java
44.77% <62.06%> (ø)

...ot/controller/helix/core/util/ZKMetadataUtils.java
76.59% <66.66%> (ø)

...n/java/org/apache/pinot/common/utils/URIUtils.java
66.66% <100.00%> (ø)

...in/java/org/apache/pinot/spi/utils/StringUtil.java
0.00% <0.00%> (-100.00%)
:arrow_down:

.../java/org/apache/pinot/spi/utils/BooleanUtils.java
0.00% <0.00%> (-100.00%)
:arrow_down:

...java/org/apache/pinot/spi/trace/BaseRecording.java
0.00% <0.00%> (-100.00%)
:arrow_down:

...java/org/apache/pinot/spi/trace/NoOpRecording.java
0.00% <0.00%> (-100.00%)
:arrow_down:

... and 1483 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 088da3f...7ce35ef. Read the comment docs.

authors: Jackie-Jiang codecov-commenter


text:
[Doc] Search doesn't work on pulsar website

Search before asking

[X] I searched in the issues and found nothing similar.

What issue do you find in Pulsar docs?
As of 2024-06-09, search bar of https://pulsar.apache.org/ doesn't show any result.

On the other hand, the search does work for https://pulsar.apache.org/docs/3.2.x/. Latest version is not indexed in Algolia somehow?
What is your suggestion?
Fix search functionality to work
Any reference?
No response
Are you willing to submit a PR?

[ ] I'm willing to submit a PR!

This seems to be fixed now.
It was probably caused when 3.3.x version was added. The Pulsar Site seems to require a full rebuild in that case. Yesterday I did the full rebuild by committing an empty commit with a specific commit message to trigger the rebuild.
git commit -m 'Rebuild: BUILD_ALL_VERSION=1' --allow-empty

the build took about 1h 20mins: https://github.com/apache/pulsar-site/actions/runs/9454073305/job/26040821948

Thank you. Confirmed it now works.

source: gharchive/issue

authors: lhotari ocadaruma

repo: apache/pulsar


text:
[Bug][meta] Broker enter the loop to recreate zkSession failed, cause broker stuck

Search before asking

[X] I searched in the issues and found nothing similar.

Read release policy

[X] I understand that unsupported versions don't get bug fixes. I will attempt to reproduce the issue on a supported version of Pulsar client and Pulsar broker.

Version
zookeeper: 3.6.3
broker: 3.0.4
bookkeeper: 4.16.4
Minimal reproduce step
continue to do perf-produce to a 5-nodes cluster, with several broker restart operation.
What did you expect to see?
broker should reCreate zkSession success and not stuck.
What did you see instead?
Firstly, everything is ok. After running for some time, notice one of the five broker have no throughput, but broker process is alive.
Then it is found in log, the broker always retry recreate zkSession frequently but failed.
15:55:28.066 [ZKC-connect-executor-0-SendThread(ip3:2181)] INFO  org.apache.zookeeper.ClientCnxn - Session establishment complete on server ip3/ip3:2181, session id = 0x3016bf7f9583a32, negotiated timeout = 30000
15:55:28.066 [ZKC-connect-executor-0-EventThread] INFO  org.apache.bookkeeper.zookeeper.ZooKeeperWatcherBase - ZooKeeper client is connected now.
15:55:28.066 [metadata-store-38-1] INFO  org.apache.pulsar.metadata.impl.ZKSessionWatcher - Got ZK session watch event: WatchedEvent state:SyncConnected type:None path:null zxid: -1
15:55:28.066 [metadata-store-38-1] INFO  org.apache.pulsar.metadata.impl.ZKSessionWatcher - ZooKeeper client reconnection with server quorum. Current status: SessionLost
15:55:28.066 [metadata-store-38-1] INFO  org.apache.pulsar.broker.PulsarService - Received metadata service session event: Reconnected
15:55:28.066 [ZKC-connect-executor-0-EventThread] ERROR org.apache.pulsar.metadata.impl.ZKMetadataStore - Failed to recreate persistent watch on ZooKeeper: CONNECTIONLOSS
15:55:28.066 [ZKC-connect-executor-0-EventThread] INFO  org.apache.pulsar.metadata.impl.PulsarZooKeeperClient - ZooKeeper session 3016bf7f9583a32 is expired from ip1:2181,ip2:2181,ip3:2181.
15:55:28.066 [metadata-store-38-1] INFO  org.apache.bookkeeper.mledger.impl.ManagedLedgerFactoryImpl - Received MetadataStore session event: Reconnected
15:55:28.066 [metadata-store-coordination-service-47-1] INFO  org.apache.pulsar.metadata.coordination.impl.LockManagerImpl - Metadata store connection has been re-established. Revalidating locks that were pending.
15:55:28.066 [metadata-store-coordination-service-64-1] INFO  org.apache.pulsar.metadata.coordination.impl.LockManagerImpl - Metadata store connection has been re-established. Revalidating locks that were pending.
15:55:28.066 [metadata-store-coordination-service-40-1] INFO  org.apache.pulsar.metadata.coordination.impl.LockManagerImpl - Metadata store connection has been re-established. Revalidating locks that were pending.
15:55:28.172 [ZKC-connect-executor-0] INFO  org.apache.zookeeper.ZooKeeper - Session: 0x3016bf7f9583a32 closed
15:55:28.406 [ZKC-connect-executor-0] INFO  org.apache.zookeeper.ZooKeeper - Initiating client connection, connectString=ip1:2181,ip2:2181,ip3:2181 sessionTimeout=30000 watcher=org.apache.bookkeeper.zookeeper.ZooKeeperWatcherBase@1a78dacd
15:55:28.173 [ZKC-connect-executor-0-EventThread] INFO  org.apache.zookeeper.ClientCnxn - EventThread shut down for session: 0x3016bf7f9583a32

15:55:28.186 [ZKC-connect-executor-0-SendThread(ip2:2181)] INFO  org.apache.zookeeper.ClientCnxn - Session establishment complete on server ip2/ip2:2181, session id = 0x20d399292970a9d, negotiated timeout = 30000
15:55:28.186 [ZKC-connect-executor-0-EventThread] INFO  org.apache.bookkeeper.zookeeper.ZooKeeperWatcherBase - ZooKeeper client is connected now.
15:55:28.186 [metadata-store-38-1] INFO  org.apache.pulsar.metadata.impl.ZKSessionWatcher - Got ZK session watch event: WatchedEvent state:SyncConnected type:None path:null zxid: -1
15:55:28.186 [metadata-store-38-1] INFO  org.apache.pulsar.metadata.impl.ZKSessionWatcher - ZooKeeper client reconnection with server quorum. Current status: SessionLost
15:55:28.186 [metadata-store-38-1] INFO  org.apache.pulsar.broker.PulsarService - Received metadata service session event: Reconnected
15:55:28.186 [ZKC-connect-executor-0-EventThread] ERROR org.apache.pulsar.metadata.impl.ZKMetadataStore - Failed to recreate persistent watch on ZooKeeper: CONNECTIONLOSS
15:55:28.186 [ZKC-connect-executor-0-EventThread] INFO  org.apache.pulsar.metadata.impl.PulsarZooKeeperClient - ZooKeeper session 20d399292970a9d is expired from ip1:2181,ip2:2181,ip3:2181.
15:55:28.186 [metadata-store-38-1] INFO  org.apache.bookkeeper.mledger.impl.ManagedLedgerFactoryImpl - Received MetadataStore session event: Reconnected
15:55:28.186 [metadata-store-coordination-service-47-1] INFO  org.apache.pulsar.metadata.coordination.impl.LockManagerImpl - Metadata store connection has been re-established. Revalidating locks that were pending.
15:55:28.186 [metadata-store-coordination-service-40-1] INFO  org.apache.pulsar.metadata.coordination.impl.LockManagerImpl - Metadata store connection has been re-established. Revalidating locks that were pending.
15:55:28.186 [metadata-store-coordination-service-64-1] INFO  org.apache.pulsar.metadata.coordination.impl.LockManagerImpl - Metadata store connection has been re-established. Revalidating locks that were pending.
15:55:28.292 [ZKC-connect-executor-0] INFO  org.apache.zookeeper.ZooKeeper - Session: 0x20d399292970a9d closed
15:55:28.292 [ZKC-connect-executor-0] INFO  org.apache.zookeeper.ZooKeeper - Initiating client connection, connectString=ip1:2181,ip2:2181,ip3:2181 sessionTimeout=30000 watcher=org.apache.bookkeep
er.zookeeper.ZooKeeperWatcherBase@1a78dacd
15:55:28.294 [ZKC-connect-executor-0-EventThread] INFO  org.apache.zookeeper.ClientCnxn - EventThread shut down for session: 0x20d399292970a9d

15:55:28.300 [ZKC-connect-executor-0-SendThread(ip3:2181)] INFO  org.apache.zookeeper.ClientCnxn - Session establishment complete on server ip3/ip3:2181, session id = 0x3016bf7f9583a33, negotiated timeout = 30000
15:55:28.300 [ZKC-connect-executor-0-EventThread] INFO  org.apache.bookkeeper.zookeeper.ZooKeeperWatcherBase - ZooKeeper client is connected now.
15:55:28.300 [metadata-store-38-1] INFO  org.apache.pulsar.metadata.impl.ZKSessionWatcher - Got ZK session watch event: WatchedEvent state:SyncConnected type:None path:null zxid: -1
15:55:28.300 [metadata-store-38-1] INFO  org.apache.pulsar.metadata.impl.ZKSessionWatcher - ZooKeeper client reconnection with server quorum. Current status: SessionLost
15:55:28.300 [metadata-store-38-1] INFO  org.apache.pulsar.broker.PulsarService - Received metadata service session event: Reconnected
15:55:28.300 [ZKC-connect-executor-0-EventThread] ERROR org.apache.pulsar.metadata.impl.ZKMetadataStore - Failed to recreate persistent watch on ZooKeeper: CONNECTIONLOSS
15:55:28.300 [ZKC-connect-executor-0-EventThread] INFO  org.apache.pulsar.metadata.impl.PulsarZooKeeperClient - ZooKeeper session 3016bf7f9583a33 is expired from ip1:2181,ip2:2181,ip3:2181.
15:55:28.300 [metadata-store-38-1] INFO  org.apache.bookkeeper.mledger.impl.ManagedLedgerFactoryImpl - Received MetadataStore session event: Reconnected
15:55:28.300 [metadata-store-coordination-service-47-1] INFO  org.apache.pulsar.metadata.coordination.impl.LockManagerImpl - Metadata store connection has been re-established. Revalidating locks that were pending.
15:55:28.300 [metadata-store-coordination-service-40-1] INFO  org.apache.pulsar.metadata.coordination.impl.LockManagerImpl - Metadata store connection has been re-established. Revalidating locks that were pending.
15:55:28.300 [metadata-store-coordination-service-64-1] INFO  org.apache.pulsar.metadata.coordination.impl.LockManagerImpl - Metadata store connection has been re-established. Revalidating locks that were pending.
15:55:28.406 [ZKC-connect-executor-0] INFO  org.apache.zookeeper.ZooKeeper - Session: 0x3016bf7f9583a33 closed
15:55:28.406 [ZKC-connect-executor-0] INFO  org.apache.zookeeper.ZooKeeper - Initiating client connection, connectString=ip1:2181,ip2:2181,ip3:2181 sessionTimeout=30000 watcher=org.apache.bookkeeper.zookeeper.ZooKeeperWatcherBase@1a78dacd
15:55:28.408 [ZKC-connect-executor-0-EventThread] INFO  org.apache.zookeeper.ClientCnxn - EventThread shut down for session: 0x3016bf7f9583a33

There is some questions.

3-node Zk server is all runnable, it seems everything of zk server is OK. Actually the other 4 broker is OK, only 1 broker enter the loop to recreate ZkSession. And all Brokers located machine's load are low. I guess restart broker can fix this issue, but I do not do that, for the reason to preserve the problem site.  But actually broker should recover by itself.
Which case would result in this error log "Failed to recreate persistent watch on ZooKeeper: CONNECTIONLOSS" ?
Currently I prefer that this stuck is because of broker's way to reCreate zk session, but not zk itself.
I don't know whether should I update the zk version with pulsar version 3.0.

Anything else?
No response
Are you willing to submit a PR?

[ ] I'm willing to submit a PR!

Now I guess the issue is relevant of the config zookeeperSessionExpiredPolicy. Pulsar new version change zookeeperSessionExpiredPolicy default value from shutdown to reconnect. https://github.com/apache/pulsar/pull/13341
When I revert the config to shutdown, zk and pulsar cluster become more stable. And the issue currently not happen.


text:
Optimize if statement

Motivation
Some  if statement need optimize, reduce code.
Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (yes / no) no
The public API: (yes / no) no
The schema: (yes / no / don't know) no
The default values of configurations: (yes / no) no
The wire protocol: (yes / no) no
The rest endpoints: (yes / no) no
The admin cli options: (yes / no) no
Anything that affects deployment: (yes / no / don't know) no

Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?

[ ] doc-required
(If you need help on updating docs, create a doc issue)

[x] no-need-doc

no new features.

[ ] doc
(If this PR contains doc changes)

/pulsarbot run-failure-checks

/pulsarbot run-failure-checks

@merlimat  pTAL again.


text:
[integration] Add messaging etcd integration tests

Fixes #14295
Motivation
Adding Ectd integration tests #14295
Modifications
Adding Ectd integration tests
Verifying this change

[x] Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

Adding Ectd integration tests

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation
Check the box below or label this PR directly (if you have committer privilege).
Need to update docs?

[ ] doc-required

[x] no-need-doc

[ ] doc

There is a remaining problem, the bug of bookkeeper 4.14.4 will cause the ci to fail, and it needs to be solved
issue: https://github.com/apache/bookkeeper/pull/3011


text:
[refactor][c++ client] Enable client memory limit controller by default.

Motivation
#13306
Modifications

Enable C++ client memory limit controller by default.
Git ignore .cmake folder

Documentation

[x] doc-not-needed
(Please explain why)

Currently we cannot disable maxPendingMessages for C++ client, see the failed tests. In addition, C++ client doesn't implement memory limit for consumer. We need to implement PIP-74 for C++ client before that.
Even for producer, there was an issue for memory limit, see #11910. You can verify this issue first.

I get it, thanks. I will perfect the memory limit first.


text:
[improve][broker] Added RoundRobinBrokerSelector

Motivation
Added a simple RoundRobin logic to select brokers for bundle ownership. This simple selector can help distribute the bundle load when there are many concurrent lookup requests(especially useful for performance tests where most lookups happen at the beginning of tests).
Modifications
Added RoundRobinBrokerSelector and its unit test.
Verifying this change

[x] Make sure that the change passes the CI checks.

(Please pick either of the following options)
This change added tests and can be verified as follows:

Added an unit test to cover RoundRobinBrokerSelector

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

[ ] Dependencies (add or upgrade a dependency)
[ ] The public API
[ ] The schema
[ ] The default values of configurations
[ ] The threading model
[ ] The binary protocol
[ ] The REST endpoints
[ ] The admin CLI options
[ ] The metrics
[ ] Anything that affects deployment

Documentation

[ ] doc
[ ] doc-required
[x] doc-not-needed
[ ] doc-complete

Matching PR in forked repository
PR in forked repository: https://github.com/heesung-sn/pulsar/pull/22

Codecov Report

Merging #19321 (c18170f) into master (8d81392) will increase coverage by 0.31%.
The diff coverage is 54.34%.

@@             Coverage Diff              @@
##             master   #19321      +/-   ##
============================================
+ Coverage     63.93%   64.24%   +0.31%
- Complexity     3470    26018   +22548
============================================
  Files          1818     1819       +1
  Lines        133061   133132      +71
  Branches      14637    14649      +12
============================================
+ Hits          85067    85529     +462
+ Misses        40270    39797     -473
- Partials       7724     7806      +82

Flag
Coverage Δ

inttests
24.92% <0.00%> (-0.03%)
:arrow_down:

systests
25.56% <0.00%> (-0.15%)
:arrow_down:

unittests
61.39% <54.34%> (+0.07%)
:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

...ker/loadbalance/impl/RoundRobinBrokerSelector.java
0.00% <0.00%> (ø)

...er/authentication/OneStageAuthenticationState.java
100.00% <100.00%> (ø)

...va/org/apache/pulsar/broker/service/ServerCnx.java
53.62% <100.00%> (-1.30%)
:arrow_down:

.../apache/pulsar/websocket/data/ConsumerCommand.java
0.00% <0.00%> (-100.00%)
:arrow_down:

.../impl/auth/oauth2/AuthenticationFactoryOAuth2.java
0.00% <0.00%> (-88.89%)
:arrow_down:

...a/org/apache/pulsar/client/util/TypeCheckUtil.java
0.00% <0.00%> (-83.34%)
:arrow_down:

...apache/pulsar/broker/web/MaxRequestSizeFilter.java
0.00% <0.00%> (-82.36%)
:arrow_down:

...pache/pulsar/broker/tools/GenerateDocsCommand.java
0.00% <0.00%> (-77.28%)
:arrow_down:

.../pulsar/broker/service/SharedConsumerAssignor.java
5.55% <0.00%> (-74.08%)
:arrow_down:

...ava/org/apache/pulsar/broker/tools/BrokerTool.java
0.00% <0.00%> (-71.43%)
:arrow_down:

... and 202 more

authors: codecov-commenter heesung-sn


text:
[fix][broker]A failed consumer/producer future in ServerCnx can never be removed

Motivation
Conditions under which problems occur

Failover/Exclusive subscription
Sockets of Pulsar IO are not stable
ServerCnx thread is busy.

time
registering consumer 1st
registering consumer 2nd

1
Registered

2
Socket is unstable

3
Closing ServerCnx

4

Retry to subscribe

5

Check whether the previous consumer is still active[1], if not,  override it by the current registering

6
ServerCnx is closed

7

Call serverCnx.checkConnectionLiveness, the Completable Future returned will never be completed

8

Client-side: call ClientCnx.removeConsumer after the subscribing is timeout

9

There is a failed future in ServerCnx.consumers

[1]: AbstractDispatcherSingleActiveConsumer.java
 public synchronized CompletableFuture<Void> addConsumer(Consumer consumer) {

  if (getActiveConsumer() != null) {
      return actConsumer.cnx().checkConnectionLiveness().thenCompose(actConsumerStillAlive -> {
          if (actConsumerStillAlive.isEmpty() || actConsumerStillAlive.get()) {
              return FutureUtil.failedFuture(new ConsumerBusyException("Exclusive consumer is already"
                      + " connected"));
          } else {
              return addConsumer(consumer);
          }
      });
  }
}

Affects
It leads to the following subscribing can never succeed due to the ServerCnx maintaining a failed future and it can never be removed.
Error logs
2024-07-25T09:28:00,109+0000 [pulsar-io-4-30] WARN  org.apache.pulsar.broker.service.ServerCnx - [/127.0.0.2:48345][persistent://{tenant}/{namespace}/__change_events-partition-0][multiTopicsReader-381f38cdef] A failed consumer with id is already present on the connection, consumerId=7425
2024-07-25T09:28:00,109+0000 [pulsar-io-4-30] ERROR org.apache.pulsar.broker.service.ServerCnx - A failed consumer with id is already present on the connection. consumerId: 7425, remoteAddress: /127.0.0.2:48345, subscription: multiTopicsReader-381f38cdef
...
...
...
// loop

Modifications
Fix the bug.
Documentation

[ ] doc
[ ] doc-required
[x] doc-not-needed
[ ] doc-complete

Matching PR in forked repository
PR in forked repository: x

/pulsarbot rerun-failure-checks

Codecov Report
Attention: Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.

Project coverage is 73.46%. Comparing base (bbc6224) to head (d87364d).
Report is 494 commits behind head on master.

Files
Patch %
Lines

...va/org/apache/pulsar/broker/service/ServerCnx.java
53.84%
4 Missing and 2 partials :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #23123      +/-   ##
============================================
- Coverage     73.57%   73.46%   -0.11%
- Complexity    32624    33223     +599
============================================
  Files          1877     1919      +42
  Lines        139502   144131    +4629
  Branches      15299    15753     +454
============================================
+ Hits         102638   105887    +3249
- Misses        28908    30116    +1208
- Partials       7956     8128     +172

Flag
Coverage Δ

inttests
27.85% <30.76%> (+3.26%)
:arrow_up:

systests
24.78% <0.00%> (+0.46%)
:arrow_up:

unittests
72.51% <53.84%> (-0.33%)
:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files
Coverage Δ

...va/org/apache/pulsar/broker/service/ServerCnx.java
72.23% <53.84%> (+0.09%)
:arrow_up:

... and 518 files with indirect coverage changes

authors: codecov-commenter poorbarcode


text:
[Issue: 3161][Go client] Return Message ID for send

Signed-off-by: xiaolong.ran ranxiaolong716@gmail.com
Fixes #3161 #3180 #4863
Motivation
The Send(context.Context, ProducerMessage) error should return MessageID.
 SendAsync(context.Context, ProducerMessage, func(ProducerMessage, error))

In here, we shouldn't return ProducerMessage, because the value is nil.

@sijie @jiazhai @merlimat PTAL

I think the change looks good to me, and it makes the API of C++/Go/Python consistent with Java. However since this change change the callback signatures, it would be great to see the opinions from cpp client users.
@merlimat @rdhabalia @massakam can you guys please take a look at this change?

ping @merlimat @rdhabalia @massakam ?

hello @merlimat @rdhabalia @massakam PTAL thanks.

Png @merlimat @rdhabalia @massakam If time is convenient, please review the pull request, thanks.

Ping @merlimat @rdhabalia @massakam If time is convenient, please review the pull request, thanks again.

Ping @merlimat @rdhabalia @massakam If time is convenient, please review the pull request, thanks again.

Ping @merlimat @rdhabalia @massakam

Sorry for the late response. This change looks good to me too.

Sorry for the late response. @massakam PTAL again thanks.

ping @massakam @merlimat PTAL

@massakam PTAL again thanks

ping @massakam PTAL

ping @massakam PTAL again thanks

rerun java8 tests
rerun cpp tests

rerun java8 tests

rerun java8 tests

rerun java8 tests

rerun java8 tests

rerun java8 tests

@merlimat What do you think about this change?

ping @merlimat PTAL

Log here for posterity, pre 2.4 python client e.g. 2.3.2 would segfault if people use send_async and try to inspect msg.top_name() etc in the call back. 2.4+ python client would correctly complain about attribute errors on MessageId object.

authors: massakam sijie vicaya wolfstudy


text:
[build] Skip javadoc task for pulsar-client-kafka-compact modules

Motivation
pulsar-client-kafka-compact depends on pulsar-client implementation hence it pulls in
protobuf dependencies. This results in class file for com.google.protobuf.GeneratedMessageV3 not found
errors when generating javadoc for those modules.
Modifications
Skip javadoc tasks for these modules. Because:

pulsar-client-kafka-compact is a kafka wrapper. Kafka already provides javadoc for this API.
we didn't publish the javadoc for this module.

retest this please

retest this please

retest this please

run java8 tests
run integration tests


text:
Update Pulsar related events

Motivation
The Events page is not updated.
Modifications
Update Pulsar related events.

It seems that we could not fill date in the calendar. So what's the purpose of the calendar here?

/pulsarbot run-failure-checks


text:
[feature][python-client]support python end to end encryption

Motivation
After supporting end-to-end encryption for nodejs, consider supporting end-to-end encryption for python clients.
Modifications

Add static method create for DefaultCryptoKeyReader
Add ConsumerConfiguration_setCryptoKeyReader and ProducerConfiguration_setCryptoKeyReader
Add class CryptoKeyReader
Add python integration test

Verifying this change

Add test

(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API: (yes / no)
The schema: (yes / no / don't know)
The default values of configurations: (yes / no)
The wire protocol: (yes / no)
The rest endpoints: (yes / no)
The admin cli options: (yes / no)
Anything that affects deployment: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
If a feature is not applicable for documentation, explain why?
If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@BewareMyPower Can you also review this?

cpp tls test failed
Handshake failed: certificate verify failed
Which is: Ok
[  FAILED  ] AuthPluginTest.testTls (111 ms)
[----------] 1 test from AuthPluginTest (111 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (111 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] AuthPluginTest.testTls

[  FAILED  ] AuthPluginTest.testTlsDetectPulsarSsl (128 ms)
[----------] 1 test from AuthPluginTest (128 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (128 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] AuthPluginTest.testTlsDetectPulsarSsl

/pulsarbot run-failure-checks

@merlimat PTAL

/pulsarbot run-failure-checks

/pulsarbot run-failure-checks

ping @merlimat PTAL

authors: sijie tuteng


text:
[ISSUE #8065] Increase tiered storage destroy expired files interval to 30min

Which Issue(s) This PR Fixes

Fixes #8065
Brief Description

See #8065
How Did You Test This Change?

Codecov Report
Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 42.89%. Comparing base (9c8fdb7) to head (e60e21f).

Files
Patch %
Lines

...pache/rocketmq/tieredstore/file/FlatFileStore.java
0.00%
1 Missing :warning:

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #8217      +/-   ##
=============================================
- Coverage      42.93%   42.89%   -0.05%
+ Complexity     10377    10369       -8
=============================================
  Files           1270     1270
  Lines          88727    88727
  Branches       11407    11407
=============================================
- Hits           38095    38057      -38
- Misses         45936    45964      +28
- Partials        4696     4706      +10

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

可以修改成 1 个小时，因为保留时间基本是按照小时为单位设计的，Init delay 也可以是 1 小时。rebase 下 master

authors: bxfjb codecov-commenter lizhimins

repo: apache/rocketmq


text:
Santize some inputs and clarify docs

This PR sanitizes a couple of inputs and adds some docs clarifications.

@mbien thanks for the review

repo: apache/roller


text:
[Question] How to use spring.profiles.active parameter to startup

Question
This is how to startup in web site.

like this, when I used this paramter, but it didn't active, it running with default profiles, I think in start.sh should has a paramter after ${MAIN_CLASS}.

And, when I used this shell, I met a error of The stack size specified is too small, Specify at least 384k. so I edit -Xss to startup. It was my machine mistask?

Sorry for my late reply.
The first question, I think there is a problem with our documents. If I want to use mysql or other databases, it is recommended to modify the configuration files in conf/.
The second problem, this is a problem caused by the environment, this is related to StackYellowPage, StackRedPage, StackShadowPage, OS page size and VM default page size(8k). Therefore, I suggest that in version 2.5.0 and earlier, according to your own env to modify the startup script according to the actual situation. After 2.5.1, the startup script will support customizing jvm parameters through environment variables.

Thank for your replay.
About the first question I deineded variable of active profiles, and added this after ${MAIN_CLASS} in start.sh to use different propertis file.

authors: JooKS-me MiracleDx

repo: apache/shenyu


text:
[Task] Remove macos CI flow

Description
Remove macos CI flow because of the inefficiency
Task List
No response

assign me, pls


text:
[type:refactor] refactor shenyu-client-tars.

about #3932
[type:refactor] refactor shenyu-client-tars.

Make sure that:

[ ] You have read the contribution guidelines.
[ ] You submit test cases (unit or integration tests) that back your changes.
[ ] Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

Codecov Report

Merging #3938 (b6a83bf) into master (3995940) will decrease coverage by 0.07%.
The diff coverage is 77.41%.

@@             Coverage Diff              @@
##             master    #3938      +/-   ##
============================================
- Coverage     70.35%   70.28%   -0.08%
+ Complexity     6748     6734      -14
============================================
  Files           922      921       -1
  Lines         25318    25294      -24
  Branches       2297     2291       -6
============================================
- Hits          17813    17778      -35
- Misses         6136     6143       +7
- Partials       1369     1373       +4

Impacted Files
Coverage Δ

...ter/client/tars/ShenyuTarsClientConfiguration.java
100.00% <ø> (ø)

...enyu/client/tars/TarsServiceBeanEventListener.java
86.58% <77.41%> (-6.01%)
:arrow_down:

...controller/ShenyuClientHttpRegistryController.java
77.77% <0.00%> (-22.23%)
:arrow_down:

...ruptor/RegisterClientServerDisruptorPublisher.java
52.94% <0.00%> (-11.77%)
:arrow_down:

...shenyu/sync/data/consul/ConsulSyncDataService.java
78.33% <0.00%> (-1.67%)
:arrow_down:

...henyu/plugin/grpc/resolver/ShenyuNameResolver.java
62.76% <0.00%> (-1.07%)
:arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

authors: codecov-commenter yunlongn


text:
skywalking dashboard and tracing page is blank when startup by docker-compose

Please answer these questions before submitting your issue.

Why do you submit this issue?
[x] Question or discussion
[ ] Bug
[ ] Requirement
[ ] Feature or performance improvement

Question

What do you want to know?

I want setup a playgroud environment by docker.

using docker-compose.yml
emit a request to my demo application that start with Java agent

The question is both dashboard and tracing page is blank in UI. And instance_name is empty.

But it can return data by a specific trace id that i take from log

I also query the instance from skywalking-cli. It's correct
/bin/swctl-0.5.0-darwin-amd64 service ls test-application
[{"id":"dGVzdC1hcHBsaWNhdGlvbg==.1","name":"test-application","group":""}

./bin/swctl-0.5.0-darwin-amd64 instance ls --service-name=test-application
[{"id":"dGVzdC1hcHBsaWNhdGlvbg==.1_OWQ0YTNmMzk2NzFkNGMxMWJjYTEzYWQ4NzY3ZmM4MmNAMTkyLjE2OC44LjY3","name":"9d4a3f39671d4c11bca13ad8767fc82c@192.168.8.67","attributes":[{"name":"OS Name","value":"Mac OS X"},{"name":"hostname","value":"v-zhidus-MacBook-Pro.local"},{"name":"Process No.","value":"32235"},{"name":"ipv4s","value":"192.168.8.67"}],"language":"JAVA","instanceUUID":"dGVzdC1hcHBsaWNhdGlvbg==.1_OWQ0YTNmMzk2NzFkNGMxMWJjYTEzYWQ4NzY3ZmM4MmNAMTkyLjE2OC44LjY3"}]

There is no problem without docker. Can you help me with this question? Am i miss something important ?

Bug

Which version of SkyWalking, OS and JRE?

Which company or project?

What happened?
If possible, provide a way to reproduce the error. e.g. demo application, component version.

Requirement or improvement

Please describe your requirements or improvement suggestions.

Because docker is using UTC-0 as TZ. Please select correct timezone at the UI.

authors: v-zhidu wu-sheng

repo: apache/skywalking

license_source: bigquery


text:
Remove dashboard json header

[ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
[ ] Update the CHANGES log.

@smartboy37597 I have asked @kezhenxu94 to check what happens on the CI, most likely due to volume space issues.
Your PR should be good once that geta fixed.


text:
Remove invalid mysql configuration in agent.config.

Remove invalid mysql configuration in agent.config.

Good catch, please update the change log too.


text:
SOLR-16916: JSON boolean queries when solrconfig defType is set to edismax

https://issues.apache.org/jira/browse/SOLR-16916
Description
As discussed on this solr-users thread, if your requestHandler has a default defType of "edismax", you are unable to send boolean queries using the JSON Query DSL syntax.  This PR resolves the issue.
Solution
We took the approach recommended by @dsmiley in the jira ticket (thanks, @dsmiley !)
Tests
We created a new JUnit test based on the existing JSON Query tests, but with a different solrconfig.xml.  We confirmed that without our fix, the test fails, and with the fix, the test passes.
Checklist
Please review the following and check all that apply:

[x] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
[x] I have created a Jira issue and added the issue ID to my pull request title.
[ ] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
[x] I have developed this patch against the main branch.
[x] I have run ./gradlew check.
[x] I have added tests for my changes.
[ ] I have added documentation for the Reference Guide

I can anticipate a problem with my suggestion; may or may not be an issue.  Sometimes tests use Solr home's and/or configSets in-place (end up being in a build dir) in directories that we consider read-only.  Tests that manipulate configuration need to use a temp directory.  Gradle executed tests enforce the read-only rule but using your IDE will probably not enforce it.  Other than copying files to a temp dir, I don't have a simpler recommendation.  There's no hurry on this; I can try to work on it being easier.  Ultimately I want this to be the norm for tests.

Instructions:  https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests
Somehow I think it can be set to automatic so you don't have to think about it henceforth, but I'm not sure.
About some changes I did:

Don't use SolrTestCaseHS at all, even the assertJQ thing.  There is only one trivial assert so it's easy to have it assert the manual way.  Still... it's curious that the JSON path expression didn't work.  I dug into that but it's a distraction here.
The configSet needs to be a copy because we modify it.  The ~3 lines for this are kinda ugly but it's for another time to improve it.
Using a very minimal configSet.

There is still some tidying up for you to do based on my comments.  I also think the test data should be simplified because your test doesn't even use half of it.

Thanks for all your help, @dsmiley !  The test is so much more readable now!  Should be ready for your re-review.

I switched the test to subclass SolrTestCaseJ4 (which 99% of Solr tests ultimately extend from) as there were mysterious cross-test side-effects when I didn't.  I don't have time today to debug why that is the case.  The recent test run still shows one failure for org.apache.solr.search.TestCoordinatorRole  but it's a known flapper as indicated by http://fucit.org/solr-jenkins-reports/failure-report.html
I'll add a CHANGES.txt entry crediting you and merge fairly soon.  Proposed text:

SOLR-16916: Use of the JSON Query DSL should ignore the defType parameter (Jane Sandberg, David Smiley)

Thank you, @dsmiley !  Could you please change "(Jane Sandberg, David Smiley)" to "(Christina Chortaria, Max Kadel, Ryan Laddusaw, Jane Sandberg, David Smiley)"?  All four of us here worked on it.

Just a note to flag the comment on the jira issue that was left a couple of days ago. This seems to have broken some functionality in solr 9.4 (https://issues.apache.org/jira/browse/SOLR-16916)

authors: dsmiley noah-dbc sandbergja

repo: apache/solr


text:
[SPARK-12177] [STREAMING] Update KafkaDStreams to new Kafka 0.9 Consumer API

…mer API

Can one of the admins verify this patch?

authors: AmplabJenkins ntarasenko

repo: apache/spark


text:
[SPARK-12934][SQL] Count-min sketch serialization

This PR adds serialization support for CountMinSketch.
A version number is added to version the serialized binary format.

Test build #49967 has started for PR 10893 at commit e97d7f9.

Test build #49967 has finished for PR 10893 at commit e97d7f9.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
      throw new CountMinSketchMergeException(\"Cannot merge estimator of class \" + other.getClass().getName());
public class CountMinSketchMergeException extends Exception

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/49967/
Test PASSed.

Test build #50020 has started for PR 10893 at commit 4abc4e0.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/50019/
Test FAILed.

Test build #50023 has started for PR 10893 at commit 9acfe33.

Test build #50025 has started for PR 10893 at commit 4636af1.

Test build #50023 has finished for PR 10893 at commit 9acfe33.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/50023/
Test PASSed.

Test build #50025 has finished for PR 10893 at commit 4636af1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/50025/
Test PASSed.

Test build #50020 has finished for PR 10893 at commit 4abc4e0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/50020/
Test PASSed.

LGTM overall

Going to merge this. Thanks. Please address my (one) feedback in your next pr.

authors: AmplabJenkins SparkQA cloud-fan liancheng rxin


text:
[SPARK-13616][SQL] Following PR for SPARK-13616

What changes were proposed in this pull request?
As all operators except Join should support converting back to SQL when it is top node. We should only take care of Join for this purpose.
How was this patch tested?
All existing tests should be passed.

Fix the problem mentioned by @gatorsmile in #11466.
cc @rxin @liancheng

Test build #52515 has started for PR 11537 at commit 1ae4f8b.

Hey @viirya, I just commented in #11466 and I tend to revert #11466 first and revisit it after we have full SQL generation support for all major language structures.
Another thing confusing me is that, although @gatorsmile mentioned that #11466 brought regressions, I didn't find them (Jenkins PR builder looks fine).

@liancheng FYI - I did hit a problem while working on SQL generation for Generate. I found that Limit is not in the list of cases of top nodes and my test cases with Limit clause started failing after i rebased with master.

Test build #52515 has finished for PR 11537 at commit 1ae4f8b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/52515/
Test PASSed.

@dilipbiswal Thanks, I think I'd better revert #11466 first for safe.

Let me close this now.

@liancheng I think @gatorsmile doesn't include the tests in his PR.

We have to locally run "hive/test-only org.apache.spark.sql.hive.execution.HiveCompatibilitySuite" and check the log file, as mentioned in the JIRA: https://issues.apache.org/jira/browse/SPARK-11012
Yeah, the regressions does not stop the test cases. If a logical plan is inconvertible, HiveComparisonTest falls back to its original behavior. When we can complete SQL generation for all the all the test cases in HiveCompatibilitySuite, we need to disable the auto fallback.
Definitely, we need to add more test cases to LogicalPlanToSQLSuite.

authors: AmplabJenkins SparkQA dilipbiswal gatorsmile liancheng viirya


text:
[MINOR][DOCS] Remove remaining sqlContext in documentation at examples

What changes were proposed in this pull request?
This PR removes sqlContext in examples. Actual usage was all replaced in https://github.com/apache/spark/pull/12809 but there are some in comments.
How was this patch tested?
Manual style checking.

Test build #58142 has started for PR 13006 at commit 3c530ad.

Test build #58142 has finished for PR 13006 at commit 3c530ad.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58142/
Test PASSed.

Merging into master after making the changes I suggested

@andrewor14 Ah, sorry I will.

authors: AmplabJenkins HyukjinKwon SparkQA andrewor14


text:
[BUILD] Test closing stale PRs

What changes were proposed in this pull request?
Here I'm seeing if we can close stale PRs via a PR message, as I'd expect.
See thread https://www.mail-archive.com/dev@spark.apache.org/msg14149.html
Closes #9354
Closes #9451
Closes #10507
Closes #10486
Closes #10460
Closes #10967
Closes #10681
Closes #11766
Closes #9907
Closes #10209
Closes #10379
Closes #10403
Closes #10842
Closes #11036
Closes #13003
Closes #10887
How was this patch tested?
(No changes)

Test build #58375 has started for PR 13052 at commit b81422f.

+1

Do you mind adding #13003 maybe?

Test build #58375 has finished for PR 13052 at commit b81422f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/58375/
Test FAILed.

Merged build finished. Test FAILed.

@HyukjinKwon that's already in the description. I'm ignoring the failure since there is no change to anything at all here.

Merged to master. Let's see if it worked.

PS for anyone who tries this -- it does work, but to make the merge script work with an empty commit, its git commit command needs --allow-empty. I didn't actually suggest changing the script since we generally don't want to allow that, I think.

authors: AmplabJenkins HyukjinKwon SparkQA srowen


text:
[SPARK-18750][yarn] Avoid using "mapValues" when allocating containers.

That method is prone to stack overflows when the input map is really
large; instead, use plain "map". Also includes a unit tests that was
tested and caused stack overflows without the fix.

Test build #71751 has started for PR 16667 at commit 5d33e3e.

Test build #71751 has finished for PR 16667 at commit 5d33e3e.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71751/
Test FAILed.

Argh, api not available in old hadoop... fix coming.

Test build #71756 has started for PR 16667 at commit 16a99fc.

Test build #71756 has finished for PR 16667 at commit 16a99fc.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71756/
Test PASSed.

@srowen @tgravescs

Test build #71867 has started for PR 16667 at commit 68c8925.

Test build #71867 has finished for PR 16667 at commit 68c8925.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71867/
Test PASSed.

+1

authors: AmplabJenkins SparkQA tgravescs vanzin


text:
[SPARK-18016][SQL][CATALYST][BRANCH-2.2] Code Generation: Constant Pool Limit - Class Splitting

What changes were proposed in this pull request?
This is a backport patch for Spark 2.2.x of the class splitting feature over excess generated code as was merged in #18075.
How was this patch tested?
The same test provided in #18075 is included in this patch.

Can one of the admins verify this patch?

ok to test

Test build #78393 has started for PR 18377 at commit 8998ee5.

Test build #78393 has finished for PR 18377 at commit 8998ee5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78393/
Test PASSed.

thanks, merging to 2.2!

@bdrillard @cloud-fan given that this is not a regression and 2.2 is in code-freeze, do you think this is safe to merge it in the 2.2 branch? More specifically, could the new code path affect (or regress) classes that would otherwise not grow beyond 1600k limit? Thanks!

@sameeragarwal @cloud-fan If I understand well what you mean by affect/regress, this patch would induce no change at all on the code generated for any class below the 1600k limit. For such classes smaller than the limit, all functions are inlined to the OuterClass, which the declareAddedFuctions method now references. Structurally, there would be no change to classes with code generated below the threshold. I don't know if this explanation suffices to address your concerns, but I'd say the patch is pretty passive in that code generation is only affected for cases over the 1600k threshold.

Why did we backport this? This seems too risky.

as we are very close to 2.2, and this patch is not trivial(200+ lines), I'm going to revert it and merge it back after 2.2 release, so this patch can be in 2.2.1 and we can have plenty of time to test it. cc @bdrillard what do you think?

reverted, @bdrillard ping me if you have any other concerns, thanks!

@cloud-fan Sure! That makes perfect sense to me.

Thanks guys!

Hm I'm not even sure if we should backport this in branch-2.2. Let's wait and see ...

@cloud-fan @rxin @sameeragarwal What can we say is the status of these backports? Including this PR, #18354, and #18579, it was decided to revert the changes until the 2.2.0 release was finished. Will all the backports remain reverted? Or can we find a way to get these merged? It seems some people would find backports useful (see SPARK-18016 jira discussion), but naturally I'd want to make sure the patches fit your testing requirements/release schedule.

@bdrillard Sorry, we do not plan to backport this PR to the maintenance branch 2.2. Could you please close this PR? Thanks!

authors: AmplabJenkins SparkQA bdrillard cloud-fan gatorsmile rxin sameeragarwal


text:
[SPARK-22799][ML] Bucketizer should throw exception if single- and multi-column params are both set

What changes were proposed in this pull request?
Currently there is a mixed situation when both single- and multi-column are supported. In some cases exceptions are thrown, in others only a warning log is emitted. In this discussion https://issues.apache.org/jira/browse/SPARK-8418?focusedCommentId=16275049&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16275049, the decision was to throw an exception.
The PR throws an exception in Bucketizer, instead of logging a warning.
How was this patch tested?
modified UT

Test build #84970 has started for PR 19993 at commit 8f3581c.

Test build #84970 has finished for PR 19993 at commit 8f3581c.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84970/
Test FAILed.

Jenkins, retest this please

Test build #84978 has started for PR 19993 at commit 8f3581c.

Test build #84978 has finished for PR 19993 at commit 8f3581c.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84978/
Test FAILed.

@hhbyyh thanks for the review. I see that for some classes there are ongoing PRs. Thus I cannot change them now in order to have a common place and a common test. Should I wait for those PRs to be merged then? Or were you suggesting something different?
Thanks.

I would suggest to develop the common infrastructure and unit test first, then other PR can take it or we can send follow-up fix.
cc @MLnick for advice.

@hhbyyh I created a common infrastructure. Please let me know if I have to modify it. Meanwhile, I'd like to discuss where to put the common UTs: do you have any specific idea about the right place? Thanks.

Test build #85123 has started for PR 19993 at commit bb0c0d2.

Test build #85124 has started for PR 19993 at commit 9f56800.

Test build #85123 has finished for PR 19993 at commit bb0c0d2.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85123/
Test FAILed.

Merged build finished. Test FAILed.

Test build #85124 has finished for PR 19993 at commit 9f56800.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85124/
Test FAILed.

Test build #85128 has started for PR 19993 at commit f593f5b.

Test build #85128 has finished for PR 19993 at commit f593f5b.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85128/
Test FAILed.

Test build #85130 has started for PR 19993 at commit 2ecdc73.

Test build #85130 has finished for PR 19993 at commit 2ecdc73.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85130/
Test FAILed.

Test build #85133 has started for PR 19993 at commit 26fe05e.

Test build #85133 has finished for PR 19993 at commit 26fe05e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85133/
Test PASSed.

@hhbyyh thanks for the comments. I already fixed all your comments, but I am waiting to push for the UT. Honestly I think that checkParam is not the best place. Checking the exception requires setting the parameters and invoking transform, thus I am not sure it is the best place, since it is a very generic one and at the moment we are setting no parameter there. What do you think?

To make it available for other classes, we need to support checking for both fit and transform, that means we also need a sample input Dataset, so we may have to add the explicit test in each of the test suite. But we can still create some infrastructure function for the explicit test to invoke.
E.g. we can create some function in object ParamsSuite or other places
checkMultiColumnParams(obj: Params, sampleData: Dataset[_])

Test build #85209 has started for PR 19993 at commit 9872bfd.

thanks @hhbyyh, I updated the PR according to your suggestion and previous comments.

Test build #85209 has finished for PR 19993 at commit 9872bfd.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85209/
Test PASSed.

Merged build finished. Test PASSed.

Test build #85253 has started for PR 19993 at commit d0b8d06.

Test build #85253 has finished for PR 19993 at commit d0b8d06.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85253/
Test PASSed.

Yes featurehasher doesn’t need inputCol and inputCols - since it is a new
multi column transformer. We may see this more in future - as I think new
transformers should be able to work on multiple columns if possible
On Thu, 21 Dec 2017 at 20:27, Yuhao Yang notifications@github.com wrote:

@hhbyyh commented on this pull request.
LGTM besides the one comment.
In mllib/src/main/scala/org/apache/spark/ml/param/params.scala
https://github.com/apache/spark/pull/19993#discussion_r158344862:

@@ -249,6 +250,31 @@ object ParamValidators {
def arrayLengthGt[T](lowerBound: Double): Array[T] => Boolean = { (value: Array[T]) =>
value.length > lowerBound
}

/**

Checks that either inputCols and outputCols are set or inputCol and outputCol are set. If

this is not true, an IllegalArgumentException is raised.

@param model

*/
private[spark] def checkMultiColumnParams(model: Params): Unit = {
model match {

 case m: HasInputCols with HasInputCol if m.isSet(m.inputCols) && m.isSet(m.inputCol) =>

   raiseIncompatibleParamsException("inputCols", "inputCol")

 case m: HasOutputCols with HasInputCol if m.isSet(m.outputCols) && m.isSet(m.inputCol) =>

   raiseIncompatibleParamsException("outputCols", "inputCol")

 case m: HasInputCols with HasOutputCol if m.isSet(m.inputCols) && m.isSet(m.outputCol) =>

Sorry to miss it, but I just found that FeatureHasher has both InputCols
and OutputCol.
I think we can remove the case and the one above since they can be too
strict.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/apache/spark/pull/19993#pullrequestreview-85142998,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA_SB9r4yuwTuXOunUCu22mtdBNRLtePks5tCqMcgaJpZM4RD1b4
.

Test build #85281 has started for PR 19993 at commit b20fb91.

thanks @hhbyyh, I removed the 2 too strict checks.

Test build #85281 has finished for PR 19993 at commit b20fb91.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85281/
Test PASSed.

@MLnick @viirya  @WeichenXu123 any further comments on this?

@jkbradley maybe you can also help reviewing this, thanks.

Test build #85545 has started for PR 19993 at commit 09d652d.

Test build #85545 has finished for PR 19993 at commit 09d652d.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85545/
Test FAILed.

authors: AmplabJenkins MLnick SparkQA hhbyyh mgaido91


text:
[MINOR][BUILD] Fix Java linter errors

What changes were proposed in this pull request?
This PR cleans up a few Java linter errors for Apache Spark 2.3 release.
How was this patch tested?
$ dev/lint-java
Using `mvn` from path: /usr/local/bin/mvn
Checkstyle checks passed.

We can see the result from Travis CI, too.

Test build #85464 has started for PR 20101 at commit 536df66.

Thank you for review and approval, @HyukjinKwon .

retest this please

Test build #85470 has started for PR 20101 at commit 536df66.

Test build #85464 has finished for PR 20101 at commit 536df66.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85464/
Test PASSed.

Test build #85470 has finished for PR 20101 at commit 536df66.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85470/
Test PASSed.

Merged to master

Thank you all!

authors: AmplabJenkins HyukjinKwon SparkQA dongjoon-hyun srowen


text:
[SPARK-29953][SS] Don't clean up source files for FileStreamSource if the files belong to the output of FileStreamSink

What changes were proposed in this pull request?
This patch prevents the cleanup operation in FileStreamSource if the source files belong to the FileStreamSink. This is needed because the output of FileStreamSink can be read with multiple Spark queries and queries will read the files based on the metadata log, which won't reflect the cleanup.
To simplify the logic, the patch only takes care of the case of when the source path without glob pattern refers to the output directory of FileStreamSink, via checking FileStreamSource to see whether it leverages metadata directory or not to list the source files.
Why are the changes needed?
Without this patch, if end users turn on cleanup option with the path which is the output of FileStreamSink, there may be out of sync between metadata and available files which may break other queries reading the path.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added UT.

cc. @zsxwing
Please also review the assumption here;

To simplify the condition, this patch assumes that if the source files belong to the FileStreamSink, the matched source path is the root of output directory for FileStreamSink. For example, suppose we provide a glob path /a/b/c// and FileStreamSource processes the file /a/b/c/d/e/f/g/file. Then we only check /a/b/c/d/e to see whether there's FileStreamSink metadata log available.

I can address the case if we would like to consider the case where metadata log is placed under subdirectory of glob path (like /a/b/c/d/e/f/_spark_metadata) or even placed under ancestor of glob path (like /a/b/_spark_metadata). I haven't address it yet because it would bring overheads, so would like to decide about a boundary and apply it afterwards.

Test build #114057 has started for PR 26590 at commit 51ef7e0.

Can one of the admins verify this patch?

Test build #114057 has finished for PR 26590 at commit 51ef7e0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/114057/
Test FAILed.

Test build #114065 has started for PR 26590 at commit 82b6c18.

Test build #114065 has finished for PR 26590 at commit 82b6c18.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/114065/
Test FAILed.

Retest this, please

Test build #114092 has started for PR 26590 at commit 82b6c18.

Test build #114092 has finished for PR 26590 at commit 82b6c18.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/114092/
Test PASSed.

To simplify the condition, this patch assumes that if the source files belong to the FileStreamSink, the matched source path is the root of output directory for FileStreamSink. For example, suppose we provide a glob path /a/b/c// and FileStreamSource processes the file /a/b/c/d/e/f/g/file. Then we only check /a/b/c/d/e to see whether there's FileStreamSink metadata log available.

As a user I may have a directory structured where /a/b/_spark_metadata exists (or even /a/_spark_metadata). Such case I would be grateful to Spark to protect me from accidental file delete/move.

As a user I may have a directory structured where /a/b/_spark_metadata exists (or even /a/_spark_metadata). Such case I would be grateful to Spark to protect me from accidental file delete/move.

Yeah that's ideal, though ideally we now have to check all subdirectories, and given their status of whether they have metadata or not could be changed, we would end up check all subdirectories of source files per a batch. We might optimize the logic to only check each directory "once" per a batch (regardless of the number of source files) but still not 100% sure it's enough lightweight.

@HeartSaVioR I think we can simply detect whether we are using MetadataLogFileIndex here: https://github.com/apache/spark/blob/ba2bc4b0e0eea0c1b6732a18cb20e61e4f693156/sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/FileStreamSource.scala#L205
We don't need to do such complicated check because for cases you are checking, we won't go through MetadataLogFileIndex so the result is not correct anyway and the user should not use such path.

@zsxwing
Ah OK got it. That's a good point - reading files in FileStreamSink output directory without metadata information is unsafe anyway.
Btw, actually I and @gaborgsomogyi considered about edge-cases which the query reads sub-directory(-ies) or ancestor with recursive option of FileStreamSink output directory, because the actual impact here is a kind of "side-effect" which "affects" other queries. It might be less problematic that the query will read the directory "incorrectly" and incorrect output will come up. The thing is, the query will also mess up the output directory as well since processed files will be cleaned up, which will also break other queries as well.
So I feel we still have to make a decision with consideration of possible side-effect; 1) try our best to prevent all known cases with (high?) costs, 2) consider these edge-cases as bad input and we don't care at all (maybe we could document it instead.) What do you think?

@HeartSaVioR Checking all the files in all the directories in each micro-batch is definitely an overkill.
Considering metadata we can have the following cases:

Metadata doesn't exist => Files created outside of Spark and starting a new Spark query intersecting with this directory should be considered error
Metadata exists in the root => Spark created it so we must use it and we can rely on that it won't be deleted
Metadata exists but not in the root => Spark created part/all of the files and such case delete/archive can mess up metadata <=> files consistency.

Only the last one is questionable what to do. Considering the possible solution complexity (globbing through the whole tree to find metadata) we can document this as configuration error. Of course if there is a relatively simple way to detect it then it would be a good idea to stop the query in advance (but at the first glance I can't find such easy way).

Checking all the files in all the directories in each micro-batch is definitely an overkill.

+1.
I think the fundamental issue is the FileIndex interface doesn't work for complicated things. There are multiple issues here. Another example: if a user is using a glob path in FileStreamSource, we always go to InMemoryFileIndex, even if there are some matched paths created by FileStreamSink. InMemoryFileIndex knowns nothing about MetadataLogFileIndex and uses its own logic to list files.
Ideally, the defending codes should be added when doing the file listing if we would like to prevent such cases because it can also prevent reading incorrect files. However, I think that's a pretty large change and probably not worth (I have not yet figured out how to make Hadoop's glob pattern codes understand MetadataLogFileIndex, maybe impossible).
Hence I suggest we just block the cleanSource option when listing files using MetadataLogFileIndex.

Test build #114262 has started for PR 26590 at commit f9dc1a4.

Test build #114263 has started for PR 26590 at commit 8d6d08b.

Thanks for the feedback. Changed the logic to check whether the source is leveraging metadata or not. Please take a look again.

Test build #114262 has finished for PR 26590 at commit f9dc1a4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/114262/
Test PASSed.

Test build #114263 has finished for PR 26590 at commit 8d6d08b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/114263/
Test PASSed.

Test build #114406 has started for PR 26590 at commit d1ec200.

Test build #114406 has finished for PR 26590 at commit d1ec200.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/114406/
Test PASSed.

@zsxwing @gaborgsomogyi I guess I addressed all review comments. Please take next round of reviews. Thanks in advance!

Test build #114741 has started for PR 26590 at commit d7ded93.

Test build #114742 has started for PR 26590 at commit fcdb9e8.

Test build #114741 has finished for PR 26590 at commit d7ded93.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/114741/
Test PASSed.

Test build #114742 has finished for PR 26590 at commit fcdb9e8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/114742/
Test PASSed.

Bump.

LGTM.
retest this please. Triggering another test since the last run was 3 days ago.

retest this, please

Test build #114918 has started for PR 26590 at commit fcdb9e8.

Test build #114918 has finished for PR 26590 at commit fcdb9e8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/114918/
Test PASSed.

Thanks! Merging to master,

Thanks all for reviewing and merging!

@zsxwing
Btw, could you please revisit the comment in #22952 when you have time so that we could fix it in time? It could be missed so I feel it's good to address sooner than later, but at least before starting 3.0.0 RC 1 vote. Thanks in advance!
https://github.com/apache/spark/pull/22952#discussion_r347673657

authors: AmplabJenkins HeartSaVioR SparkQA gaborgsomogyi zsxwing


text:
[WIP][SQL] Fix rebasing of overlapped local timestamps during daylight saving time

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25418/
Test PASSed.

Test build #120720 has started for PR 28101 at commit b761ac1.

Test build #120720 has finished for PR 28101 at commit b761ac1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120720/
Test PASSed.

Test build #120730 has started for PR 28101 at commit 1654629.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25428/
Test PASSed.

Test build #120731 has started for PR 28101 at commit 29d4599.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/25429/
Test PASSed.

Test build #120730 has finished for PR 28101 at commit 1654629.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/120730/
Test PASSed.

Test build #120731 has finished for PR 28101 at commit 29d4599.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

thanks, merging to master/3.0!

authors: AmplabJenkins MaxGekk SparkQA cloud-fan


text:
[SPARK-38559][SQL][WEBUI]Display the number of empty partitions on spark ui

What changes were proposed in this pull request?
Display the number of empty partitions on spark ui when demoting join from broadcast-hash to smj.
Before updated the ui:

After updated the ui, display the number of empty partitions:

Why are the changes needed?
When demoting join from broadcast-hash to smj, i think it is necessary to display the number of empty partitions on spark ui.
Otherwise, users might wonder why SMJ is used when joining a small table. Displaying the number of empty partitions is useful for users to understand changes to the execution plan.
Does this PR introduce any user-facing change?
No
How was this patch tested?
existing tests.

@cloud-fan Hi, could you help to review this pr? Thanks

cc @JkSelf

Can one of the admins verify this patch?

friendly ping @JkSelf Hi, could you review this pr for me? Thanks


text:
[SPARK-40271][PYTHON][TESTS][FOLLOW-UP] Make test_lit_list test pass with ANSI mode on

What changes were proposed in this pull request?
This PR is a followup of https://github.com/apache/spark/pull/37722.
Currently test_lit_list fails with the ANSI mode on as below:
======================================================================
ERROR: test_lit_list (pyspark.sql.tests.test_functions.FunctionsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/__w/spark/spark/python/pyspark/sql/tests/test_functions.py", line 979, in test_lit_list
    actual = self.spark.range(1).select(lit(test_list)).first()[0]
  File "/__w/spark/spark/python/pyspark/sql/dataframe.py", line 2639, in first
    return self.head()
  File "/__w/spark/spark/python/pyspark/sql/dataframe.py", line 2618, in head
    rs = self.head(1)
  File "/__w/spark/spark/python/pyspark/sql/dataframe.py", line 2620, in head
    return self.take(n)
  File "/__w/spark/spark/python/pyspark/sql/dataframe.py", line 1231, in take
    return self.limit(num).collect()
  File "/__w/spark/spark/python/pyspark/sql/dataframe.py", line 1137, in collect
    sock_info = self._jdf.collectToPython()
  File "/__w/spark/spark/python/lib/py4j-0.10.9.7-src.zip/py4j/java_gateway.py", line 1323, in __call__
    answer, self.gateway_client, self.target_id, self.name)
  File "/__w/spark/spark/python/pyspark/sql/utils.py", line 205, in deco
    raise converted from None
pyspark.sql.utils.IllegalArgumentException: [CAST_INVALID_INPUT] The value 'a' of the type "STRING" cannot be cast to "DOUBLE" because it is malformed. Correct the value as per the syntax, or change its target type. Use `try_cast` to tolerate malformed input and return NULL instead. If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error.

JVM stacktrace:
org.apache.spark.SparkNumberFormatException: [CAST_INVALID_INPUT] The value 'a' of the type "STRING" cannot be cast to "DOUBLE" because it is malformed. Correct the value as per the syntax, or change its target type. Use `try_cast` to tolerate malformed input and return NULL instead. If necessary set "spark.sql.ansi.enabled" to "false" to bypass this error.
	at org.apache.spark.sql.errors.QueryExecutionErrors$.invalidInputInCastToNumberError(QueryExecutionErrors.scala:168)
	at org.apache.spark.sql.catalyst.expressions.Cast.$anonfun$castToDouble$2(Cast.scala:1132)
	at org.apache.spark.sql.catalyst.expressions.Cast.buildCast(Cast.scala:578)
	at org.apache.spark.sql.catalyst.expressions.Cast.$anonfun$castToDouble$1(Cast.scala:1125)

https://github.com/apache/spark/runs/8126104258?check_suite_focus=true
This PR fixes it by explicitly disabling the configuration.
Why are the changes needed?
To make the tests pass.
Does this PR introduce any user-facing change?
No, test-only
How was this patch tested?
CI in this PR should test it out.

cc @itholic and @zhengruifeng FYI

Merged to master.

Thanks for the followup!

authors: HyukjinKwon itholic